scripted-diff: Replace deprecated C headers #14905

hebasto commented at 2:48 AM on December 10, 2018: member

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:

ISO/IEC 14882:2011, Annex C, C.3.1:

For compatibility with the Standard C library, the C++ standard library provides the 18 C headers (D.5), but their use is deprecated in C++.

ibid, Annex D, D.5:

Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope.

[ Example: The header <cstdlib> assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header <stdlib.h> assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. — end example ]

scripted-diff: Replace deprecated C headers

-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-

03b645bfdb

fanquake added the label Refactoring on Dec 10, 2018

DrahtBot commented at 3:51 AM on December 10, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14881 (Tests: Contract testing for the procedure AddTimeData by mmachicao)
#14802 (rpc: faster getblockstats using BlockUndo data by FelixWeis)
#14111 (index: Create IndexRunner class for activing indexes. by jimpo)
#14035 (Utxoscriptindex by mgrychow)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#13743 (refactor: Replace boost::bind with std::bind by ken2812221)
#13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
#13442 (Convert the 1-way SSE4 SHA256 code from asm to intrinsics by sipa)

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.

laanwj commented at 8:12 AM on December 10, 2018: member

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.

promag commented at 8:55 AM on December 10, 2018: member

Are there advantages? Otherwise NACK.

hebasto commented at 11:33 AM on December 10, 2018: member

@laanwj @promag Thank you for reviews. The only advantage of this PR is to keep the global namespace as clean as possible (PR's description has been updated with a rationale).

... they're simply indirections that include the original file.

e.g. cstring from my system:

...
#include <string.h>
...
// Get rid of those macros defined in <string.h> in lieu of real functions.
#undef memchr
#undef memcmp
#undef memcpy
... // total 22 undefs

promag commented at 2:08 PM on December 10, 2018: member

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.

MarcoFalke commented at 4:29 PM on December 10, 2018: member

Agree with @promag. Also, there have been two similar pull requests in the past:

MarcoFalke commented at 4:30 PM on December 10, 2018: member

If this would help enforcing the std:: prefix (see #13227), I'd be concept ACK, but it seems like those changes have no effect and are purely stylistic?

practicalswift commented at 7:21 PM on December 10, 2018: contributor

ACK 03b645bfdbae164bc1f9157b505255eb68af1c02

hebasto commented at 10:23 PM on December 10, 2018: member

@MarcoFalke Thank you for your review.

Also, there have been two similar pull requests in the past:

#11261

#12184

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 :)

If this would help enforcing the std:: prefix ...

You're right. This PR won't help enforcing the std:: prefix, unfortunately.

... but it seems like those changes have no effect and are purely stylistic?

IMO, using deprecated features is not a kind of stylistic or cosmetic matter.

Document the preference of "#include <cxxx>" over "#include <xxx.h>" when including C compatibility headers 0e1379a155

hebasto commented at 10:48 PM on December 10, 2018: member

@laanwj

There's nothing about C/C++ headers in the developer notes

Cherry-picked @practicalswift's 2942efa19a73841e6c947df077d9b9622002908b commit:

Added a note about the preference of #include <cxxx> in the developer notes.

practicalswift commented at 7:20 AM on December 11, 2018: contributor

utACK 0e1379a155a7e685ed5e141ad99757597f3599a9

MarcoFalke commented at 3:55 PM on December 11, 2018: member

The scripted diff looks very verbose and is not easy to review. Mind to compress it a bit?

practicalswift commented at 4:10 PM on December 11, 2018: contributor

@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"

laanwj commented at 5:36 PM on December 11, 2018: member

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.

promag commented at 5:42 PM on December 11, 2018: member

In addition to @laanwj comment, there is nothing to prevent adding back, for instance, #include <stdio.h>, which we can easily ignore while reviewing.

hebasto commented at 6:56 AM on December 12, 2018: member

@laanwj I appreciate your position.

I still don't really see the point...

The point is the compliance with C++ ISO/IEC 14882:2011 Standard.

... 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.

Yes, it is. This PR is not about 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)...

You're right. Not all headers are covered by C++ ISO/IEC 14882:2011 Standard.

... and you'll need to memorize which ones to write in the new and old style.

Is it a job for a kind of linter? @promag

In addition to @laanwj comment, there is nothing to prevent adding back, for instance, #include <stdio.h>, which we can easily ignore while reviewing.

Updated developer notes and linter rules can help.

laanwj commented at 8:19 AM on December 12, 2018: member

The point is the compliance with C++ ISO/IEC 14882:2011 Standard.

This is not in itself a goal. Look, we have 603 open issues. If you're not solving any of them, you're just creating extra work for maintainers.

hebasto commented at 8:23 AM on December 12, 2018: member

@laanwj

Look, we have 603 open issues. If you're not solving any of them, you're just creating extra work for maintainers.

Agree. Closed.

hebasto closed this on Dec 12, 2018

hebasto deleted the branch on Dec 18, 2018

MarcoFalke locked this on Sep 8, 2021