libconsensus: adapt API header to be compliant to ANSI C #28661

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202310-libconsensus-ansi_c_api_header changing 2 files +19 −18

theStack commented at 11:25 PM on October 16, 2023: contributor
libconsensus currently can't be used in projects that still use the ANSI C (=ISO C/C89/C90) standard, as the header uses single-line-comments which are only supported since C99:
```
$ echo '#include "./src/script/bitcoinconsensus.h"\nint main() {}' > consensus_test.c
$ gcc -ansi -c consensus_test.c
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:1:1: error: C++ style comments are not allowed in ISO C90
    1 | // Copyright (c) 2009-2010 Satoshi Nakamoto
      | ^
./src/script/bitcoinconsensus.h:1:1: note: (this will be reported only once per input file)
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:96:8: warning: extra tokens at end of #endif directive [-Wendif-labels]
   96 | #endif // BITCOIN_SCRIPT_BITCOINCONSENSUS_H
      |        ^
```
This PR fixed this by changing all single-line comments to /* ... */ comments instead.

I have no knowledge about who current and potential future users of this library are and if this would ever be an issue (apparently it wasn't until now), but it feels to me that the change is trivial enough to be worth it to potentially having to avoid users having to patch the header. As an alternative, we could specify the minimum needed version (i.e. C99) somewhere.
DrahtBot commented at 11:25 PM on October 16, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK TheCharlatan

Concept ACK fanquake, kristapsk, darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	fanquake, kristapsk, darosior

libconsensus: adapt API header to be compliant to ANSI C

libconsensus currently can't be used in projects that still use the ANSI
C (=C89/C90) standard, as the header uses single-comments which are only
supported since C99:

```
$ echo '#include "./src/script/bitcoinconsensus.h"\nint main() {}' > consensus_test.c
$ gcc -ansi -c consensus_test.c
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:1:1: error: C++ style comments are not allowed in ISO C90
    1 | // Copyright (c) 2009-2010 Satoshi Nakamoto
      | ^
./src/script/bitcoinconsensus.h:1:1: note: (this will be reported only once per input file)
In file included from consensus_test.c:1:
./src/script/bitcoinconsensus.h:96:8: warning: extra tokens at end of #endif directive [-Wendif-labels]
   96 | #endif // BITCOIN_SCRIPT_BITCOINCONSENSUS_H
      |        ^
```

Fix this by changing all single-line comments by /* ... */ comments instead.

939918a9ab

theStack force-pushed on Oct 16, 2023
DrahtBot added the label CI failed on Oct 16, 2023
DrahtBot removed the label CI failed on Oct 17, 2023
fanquake commented at 9:04 AM on October 17, 2023: member

Concept ACK.
kristapsk commented at 9:21 AM on October 17, 2023: contributor

Concept ACK
darosior commented at 9:27 AM on October 17, 2023: member

Concept ACK
fanquake added the label Needs backport (25.x) on Oct 17, 2023
fanquake added this to the milestone 26.0 on Oct 17, 2023
luke-jr commented at 6:37 PM on October 18, 2023: member

Concept ~0, but if we care about this, maybe we should add a test?
maflcko removed this from the milestone 26.0 on Oct 19, 2023
maflcko removed the label Needs backport (25.x) on Oct 19, 2023
maflcko commented at 3:42 PM on October 19, 2023: member

Removed from milestone for now. This is not a regression and doesn't seem to be a blocker. Could be backported to 26.1, if there is any need and this is merged.

On the change, I tend to agree with Luke. Seems odd to change something to support a new use case, but then leave out the test. Down the line it will eventually break again in the future, nullifying the reason for the change?
theStack commented at 4:03 PM on October 19, 2023: contributor

I agree that this is a "nice to have" rather than something terribly important or urgent. It's more like following best practices when providing C libraries. As for adding a test, my knowledge about the build system is close to zero and hence I'm not sure how to add this rather unusual "does this even compile?" test-case, but happy to take hints.
sipa commented at 4:56 PM on October 19, 2023: member

I'm also unsure how relevant this is... in what kind of environment do you have a C++17 compiler (to build libbitcoinconsensus) but only a C89 compiler for whatever is using it?
darosior commented at 8:09 AM on October 20, 2023: member

We ship the library compiled so i wouldn't expect users to need a C++17 compiler? No strong opinion, it just seems a trivial change with a small upside.
TheCharlatan commented at 9:53 PM on October 23, 2023: contributor

Concept ACK
bitcoin deleted a comment on Oct 23, 2023
TheCharlatan approved
TheCharlatan commented at 1:21 PM on November 3, 2023: contributor

tACK 939918a9ab67a37294c6c63cc6ef4fe109bc75ad
DrahtBot requested review from darosior on Nov 3, 2023
DrahtBot requested review from fanquake on Nov 3, 2023
fanquake commented at 6:32 PM on January 11, 2024: member

Could draft for now, dependant on the outcome of #29189?
DrahtBot added the label CI failed on Jan 17, 2024
theStack marked this as a draft on Jan 22, 2024
theStack commented at 10:49 AM on January 22, 2024: contributor

Could draft for now, dependant on the outcome of #29189?

Agree, done.
fanquake commented at 4:12 PM on February 1, 2024: member

Going to close this for now, given the deprecation / plan to remove (#29189).
fanquake closed this on Feb 1, 2024
bitcoin locked this on Jan 31, 2025