Conversation
This commit is the result of creating a file constants.sed with contents ``` s/(0x([A-F]|[0-9])+)ULL/UINT64_C(\1)/gi s/(0x([A-F]|[0-9])+)UL/UINT32_C(\1)/gi s/(([0-9])+)ULL/UINT64_C(\1)/gi s/(([0-9])+)UL/UINT32_C(\1)/gi s/(([0-9])+)LL/INT64_C(\1)/gi ``` and running `sed -E -i -f constants.sed src/*.[ch]` (and fixing custom indentation in four affected lines). Use `git grep -iE '([A-F]|[0-9])UL'` to verify the result. Moreover, this commit removes `-Wno-long-long` from CFLAGS, which is no longer needed then. It also removes `-Wno-overlength-strings`, which is apparently not needed currently either. The motivation for this change is compliance with C89 (bitcoin-core#745) for ULL/LL but also reviewability: Even though it's not relevant in the current code, I think it's confusing to use the portable uint32_t/uint64_t types but then constants of the unsigned long (long) types, which differ across targets. Fixes bitcoin-core#745.
|
I believe these macros, as well as stdint.h itself, only exist as of C99... |
|
@sipa Initially we left Another discussion is whether we want to give up the C89 requirement. If I were to start a new project, I wouldn't use C89. But here's a pragmatic view: given that made sure the entire codebase is C89 (modulo this PR), there's no reason to give this up now. It's sometimes annoying but I can live with it (and I prefer living it with over having the discussion). Things would change of course if a specific language feature not available in C89 would make the implementation of a new feature much easier for example. But I think all of this misses the main point of this PR. I think it's simply cleaner code and the right thing(TM) to use |
|
This made me look up the rules for types of constants: https://en.cppreference.com/w/c/language/integer_constant I think the majority of the constants here can simply lose all suffixes. I think only the uint64_t ones, and those where being less than 32 bits in size would be a problem. With that in mind, concept ACK, but I think this can be simplified. |
|
I honestly don't mind either way (either ditching C89 or being more strict with C89), I already got used to C89 (even though I don't like a lot of things there). |
|
Just want to note that |
Yeah. I want to do this but I'll probably say this on a lot of PRs, and this is not a high priority... Let's close this one for now. Open for grabs if anyone wants to have it. |
3 participants
This PR
ullconstants byUINT64()constants becauseullis strictly speaking not supported by C89.UINT32()for the sake of consistency both withUINT64()but also in general with theuintXX_ttypes we use (if we useuintXX_tusing the corresponding constant macros is the most natural thing to do)gen_context.cC89 compliant. (It was not but we didn't notice because it has its own CFLAGS)Please see the commit messages to see how to re-create the first commit. This will make reviewing this rather trivial.
edit: I know this touches > 300 lines but I think it will hardly annoy any existing PRs because the lines with constants are typically not touched.