Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use a macro to compute the size of arrays at compile time #18044

Merged
merged 12 commits into from
Aug 30, 2022
Merged

Use a macro to compute the size of arrays at compile time #18044

merged 12 commits into from
Aug 30, 2022

Conversation

jepler
Copy link
Contributor

@jepler jepler commented Aug 14, 2022

While commenting on #18006 I noted that the project didn't have a common macro for implementing compile time array size computation, which was instead done at each site "in the usual way".

This PR takes the change from #18006 for quantum/util.h, then uses a "semantic patch" to change sites that could be identified automatically to use the new construct. The semantic patch was simply taken from https://github.com/alterapraxisptyltd/coccinelle-coreboot/blob/master/ARRAY_SIZE.cocci -- I did not develop it myself.

Then, a few straggling items I identified were fixed manually.

I took the two added macros in util.h in the hopes that it would allow git to see that #18006 and this PR do not conflict. It's worth noting that coreboot also has a semantic patch to convert sites that should have used a CEILING-type macro, that could be fun to run next if the project deems these kinds of changes worthwhile.

@github-actions github-actions bot added core keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Aug 14, 2022
@precondition
Copy link
Contributor

Given how frequent and widespread sizeof(a)/sizeof(a[0]); is (in C codebases in general), doesn't it count it as a "standard"/conventional C idiom at this point?

@jepler
Copy link
Contributor Author

jepler commented Aug 14, 2022 •
edited
Loading

I think that's clearly for the project to decide what constitutes good style, appropriate use of idioms, etc. I totally understand if the end result is to close this PR as an undesired change.

I personally favor use of a macro because

  • The presence of "a good name" helps make code self-documenting
  • There are (relatively low probability, but can arise during refactoring) error with the 'non-macro' form.
    • In the usual form, the name of the array is repeated twice. Especially when cutting and pasting several similar lines, it could be possible to accidentally fail to match the names.
    • In another form, recognized by the semantic patch but which I don't think was present in qmk, is the style sizeof(ARR)/sizeof(T) where T is intended to be the type of *ARR. Again, it's possible for type of the array and the explicitly specified type to get out of synch
  • I'm simply used to it in other projects, and felt weird without it
  • It might be possible using a gcc extension to test for another problem, use of ARRAY_SIZE on a pointer, rather than an array. I didn't find any of these in manual inspection.

Looking further at some of the places where things were NOT converted, they fell into a couple of categories:

  • C++ source or headers
  • Code in #else branches that the semantic patch apparently thought were not taken (best guess)
  • Places where the sizeof(ARR)/sizeof(T) idiom was used and ARR and T differed in constness

I didn't locate any "actual problems" during my investigation.

@drashna drashna requested a review from a team August 15, 2022 04:58
@KarlK90
Copy link
Member

KarlK90 commented Aug 15, 2022 •
edited
Loading

It might be possible using a gcc extension to test for another problem, use of ARRAY_SIZE on a pointer, rather than an array. I didn't find any of these in manual inspection.

As QMK uses the GNU11 standard this can/should be added (in this PR).

KarlK90 and others added 11 commits August 29, 2022 06:37
@KarlK90 @jepler
Add ARRAY_SIZE and CEILING utility macros
c5469d7
@jepler
Apply a coccinelle patch to use ARRAY_SIZE
4683d9f
@jepler
fix up some straggling items
d65fb0e
@jepler
Fix 'make test:secure'
81baf06
@jepler
Enhance ARRAY_SIZE macro to reject acting on pointers
d84a258 
The previous definition would not produce a diagnostic for
```
int *p;
size_t num_elem = ARRAY_SIZE(p)
```
but the new one will.
@jepler
explicitly get definition of ARRAY_SIZE
3febf81
@jepler
Convert to ARRAY_SIZE when const is involved
64743b6 
The following spatch finds additional instances where the array is
const and the division is by the size of the type, not the size of
the first element:
```
@ rule5a using "empty.iso" @
type T;
const T[] E;
@@

- (sizeof(E)/sizeof(T))
+ ARRAY_SIZE(E)

@ rule6a using "empty.iso" @
type T;
const T[] E;
@@

- sizeof(E)/sizeof(T)
+ ARRAY_SIZE(E)
```
@jepler
New instances of ARRAY_SIZE added since initial spatch run
290dd41
@jepler
Use ARRAY_SIZE in docs (found by grep)
2632fc5
@jepler
Manually use ARRAY_SIZE
3fc9dd1 
hs_set is expected to be the same size as uint16_t, though it's made
of two 8-bit integers
@jepler
Just like char, sizeof(uint8_t) is guaranteed to be 1
3090223 
This is at least true on any plausible system where qmk is actually used.

Per my understanding it's universally true, assuming that uint8_t exists:
https://stackoverflow.com/questions/48655310/can-i-assume-that-sizeofuint8-t-1
@jepler
Copy link
Contributor Author

jepler commented Aug 29, 2022

I've updated this PR

  • applied a 2nd semantic patch for sizeof(ARR)/sizeof(T) where ARR was const
  • Manually converted sites that the semantic patch missed
  • ensured ARRAY_SIZE would fail to compile unless it was applied to an array. In particular, its application to a pointer is a compile-time error (though not a great one). This uses a gcc extension

@drashna drashna requested a review from a team August 29, 2022 19:18
KarlK90
KarlK90 approved these changes Aug 30, 2022
View reviewed changes
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort developing and applying this convenience macro across the code base!

@KarlK90 KarlK90 merged commit 9632360 into qmk:develop Aug 30, 2022
ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
@jepler @KarlK90 @ramonimbao
Use a macro to compute the size of arrays at compile time (qmk#18044)
2a5e27b 
* Add ARRAY_SIZE and CEILING utility macros

* Apply a coccinelle patch to use ARRAY_SIZE

* fix up some straggling items

* Fix 'make test:secure'

* Enhance ARRAY_SIZE macro to reject acting on pointers

The previous definition would not produce a diagnostic for
```
int *p;
size_t num_elem = ARRAY_SIZE(p)
```
but the new one will.

* explicitly get definition of ARRAY_SIZE

* Convert to ARRAY_SIZE when const is involved

The following spatch finds additional instances where the array is
const and the division is by the size of the type, not the size of
the first element:
```
@ rule5a using "empty.iso" @
type T;
const T[] E;
@@

- (sizeof(E)/sizeof(T))
+ ARRAY_SIZE(E)

@ rule6a using "empty.iso" @
type T;
const T[] E;
@@

- sizeof(E)/sizeof(T)
+ ARRAY_SIZE(E)
```

* New instances of ARRAY_SIZE added since initial spatch run

* Use `ARRAY_SIZE` in docs (found by grep)

* Manually use ARRAY_SIZE

hs_set is expected to be the same size as uint16_t, though it's made
of two 8-bit integers

* Just like char, sizeof(uint8_t) is guaranteed to be 1

This is at least true on any plausible system where qmk is actually used.

Per my understanding it's universally true, assuming that uint8_t exists:
https://stackoverflow.com/questions/48655310/can-i-assume-that-sizeofuint8-t-1

* Run qmk-format on core C files touched in this branch

Co-authored-by: Stefan Kerkmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KarlK90 KarlK90 KarlK90 approved these changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
core dependencies documentation keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jepler @precondition @KarlK90 @drashna