Improve C89 compliance

real-or-random commented at 9:57 am on April 22, 2020: contributor

This PR

replaces ull constants by UINT64() constants because ull is strictly speaking not supported by C89.
does the same for UINT32() for the sake of consistency both with UINT64() but also in general with the uintXX_t types we use (if we use uintXX_t using the corresponding constant macros is the most natural thing to do)
makes gen_context.c C89 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.

Convert large (UL/ULL/LL) integer constants to stdint.h macros

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 (#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 #745.

cdbd5638ff

Make gen_context C89 compliant, change CFLAGS_FOR_BUILD accordingly f2266e321d

real-or-random cross-referenced this on Apr 22, 2020 from issue Change SHA256 byte counter from size_t to uint64_t by real-or-random

sipa commented at 8:40 pm on April 22, 2020: contributor

I believe these macros, as well as stdint.h itself, only exist as of C99…

real-or-random commented at 10:27 am on April 23, 2020: contributor

@sipa Well, it’s true that only C >=99 requires the existence of stdint.h. But that does not prevent a C89 compiler from providing it. In fact, even C99 does not guarantee that uint32_t and uint64_t exist. So our working model is C89 with uint32_t/uint64_t support. That fits the wording in the README: “Intended to be portable to any system with a C89 compiler and uint64_t support.” (if you add the very reasonable implicit assumption that such a C compiler will also provide uint32_t).This initially confused me too, see the discussion in #568 (review) and https://stackoverflow.com/a/26502360/2725281.

Initially we left long long in, probably because noone cared. (https://github.com/bitcoin-core/secp256k1/pull/193). And I still think noone cares.

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 uint32_t for uint32_t computations and long constants for long computations. If they turn out to be equivalent, fine, but everyone looking at the code needs to think about this every time.

sipa commented at 7:55 am on May 20, 2020: contributor

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.

elichai commented at 9:01 am on June 3, 2020: contributor

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). But I also don’t know the effect of C99 on supporting embedded systems, so it feels to me the safer choice is to double down on C89, so I’m concept ACK

elichai commented at 9:39 am on June 3, 2020: contributor

Just want to note that UINTN_C will expand to uint_leastN_t and not uintN_t according to: https://en.cppreference.com/w/c/types/integer

real-or-random commented at 3:29 pm on May 8, 2023: contributor

With that in mind, concept ACK, but I think this can be simplified.

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.

real-or-random closed this on May 8, 2023

Improve C89 compliance #746