scripted-diff: Replace deprecated C headers #14905
Conversation
-BEGIN VERIFY SCRIPT-
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <assert\.h>/#include <cassert>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <errno\.h>/#include <cerrno>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <limits\.h>/#include <climits>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <math\.h>/#include <cmath>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <signal\.h>/#include <csignal>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <stdarg\.h>/#include <cstdarg>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <stddef\.h>/#include <cstddef>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <stdint\.h>/#include <cstdint>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <stdio\.h>/#include <cstdio>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <stdlib\.h>/#include <cstdlib>/g' '{}' \;
find src -not \( -path src/crypto/ctaes -prune \) -not \( -path src/leveldb -prune \) -not \( -path src/secp256k1 -prune \) -not \( -path src/univalue -prune \) \( -name "*.h" -o -name "*.cpp" \) -exec sed -i 's/#include <string\.h>/#include <cstring>/g' '{}' \;
git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
-END VERIFY SCRIPT-
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
There's nothing about C/C++ headers in the developer notes. This would have to be added first with a clear rationale first before even considering a global update. Personally I've never seen the point of the C++-specific header names, as far as I know they're simply indirections that include the original file. Many, if not most, C++ projects use the C header names. If it's only cosmetic then NACK from me. |
|
Are there advantages? Otherwise NACK. |
|
@laanwj @promag
e.g. |
|
I think that's a really small advantage, but not too bad considering it's a scripted diff. I think you could simplify the script by using multiple substitutions with only 1 sed? I'm neutral on this one. |
|
Agree with @promag. Also, there have been two similar pull requests in the past: |
|
If this would help enforcing the |
|
ACK 03b645b |
|
@MarcoFalke Thank you for your review.
I supposed that but haven't managed to find these PRs. Thank you for pointing them out. It seems another similar PR will come next year :)
You're right. This PR won't help enforcing the
IMO, using deprecated features is not a kind of stylistic or cosmetic matter. |
…when including C compatibility headers
Cherry-picked @practicalswift's 2942efa commit:
|
|
utACK 0e1379a |
|
The scripted diff looks very verbose and is not easy to review. Mind to compress it a bit? |
|
@MarcoFalke @hebasto Suggestion: HEADERS_REGEXP="assert|errno|limits|math|signal|stdarg|stddef|stdint|stdio|stdlib|string"
git grep -lE "^#include <(${HEADERS_REGEXP}\.h)>" -- ":(exclude)src/univalue/" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/crypto/ctaes/" "*.cpp" "*.h" \
| xargs sed -i -E "s/#include <(${HEADERS_REGEXP})\\.h>/#include <c\1>/g" |
|
I still don't really see the point, this is another one of those PRs that change a substantial part of the code (130 files!) without actually changing things from the user perspective. What also bothers me is that only a part of the C headers has a designated C++ replacement (say, not the BSD sockets or POSIX ones), and you'll need to memorize which ones to write in the new and old style. |
|
In addition to @laanwj comment, there is nothing to prevent adding back, for instance, |
|
@laanwj I appreciate your position.
The point is the compliance with C++ ISO/IEC 14882:2011 Standard.
Yes, it is. This PR is not about the user perspective.
You're right. Not all headers are covered by C++ ISO/IEC 14882:2011 Standard.
Is it a job for a kind of linter?
Updated developer notes and linter rules can help. |
This is not in itself a goal. |
Agree. Closed. |
This PR replaces deprecated C headers with C++ ones for all repo except subtrees to not pollute the global namespace as much as possible.
Rationale: