Any identifier starting with 2 _ is reserved for the compiler and thus must not be used.
Renamed UniValue::__pushKV to UniValue::pushKVEnd. #27822
pull Brotcrunsher wants to merge 1 commits into bitcoin:master from Brotcrunsher:pushKVUB changing 13 files +26 −26-
Brotcrunsher commented at 4:35 PM on June 4, 2023: contributor
-
DrahtBot commented at 4:35 PM on June 4, 2023: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers ACK MarcoFalke If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #27788 (rpc: Be able to access RPC parameters by name by achow101)
- #27742 ([NO MERGE] BIP331 Ancestor Package Relay by glozow)
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.
-
hebasto commented at 5:08 PM on June 4, 2023: member
See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs for changes like this.
- Brotcrunsher force-pushed on Jun 4, 2023
- DrahtBot added the label CI failed on Jun 4, 2023
- DrahtBot removed the label CI failed on Jun 4, 2023
-
maflcko commented at 8:35 AM on June 5, 2023: member
lgtm ACK acad989e67a57709dbb882c97852c2067f9dc65e
-
hebasto commented at 8:58 AM on June 5, 2023: member
Does it make sense to force such changes with the Clang's
-Wreserved-identifierdiagnostic flag? - DrahtBot added the label Needs rebase on Jun 9, 2023
-
fanquake commented at 10:38 AM on June 15, 2023: member
@Brotcrunsher are you going to rebase this?
-
maflcko commented at 11:18 AM on June 15, 2023: member
Does it make sense to force such changes with the Clang's
-Wreserved-identifierdiagnostic flag?I get a warning about
_EraseTxand this:diff --git a/src/support/allocators/secure.h b/src/support/allocators/secure.h index a0918bf463..b2076bea07 100644 --- a/src/support/allocators/secure.h +++ b/src/support/allocators/secure.h @@ -32,9 +32,9 @@ struct secure_allocator : public std::allocator<T> { { } ~secure_allocator() noexcept {} - template <typename _Other> + template <typename Other> struct rebind { - typedef secure_allocator<_Other> other; + typedef secure_allocator<Other> other; }; T* allocate(std::size_t n, const void* hint = nullptr) diff --git a/src/support/allocators/zeroafterfree.h b/src/support/allocators/zeroafterfree.h index 795eea3bc0..2dc644c242 100644 --- a/src/support/allocators/zeroafterfree.h +++ b/src/support/allocators/zeroafterfree.h @@ -27,9 +27,9 @@ struct zero_after_free_allocator : public std::allocator<T> { { } ~zero_after_free_allocator() noexcept {} - template <typename _Other> + template <typename Other> struct rebind { - typedef zero_after_free_allocator<_Other> other; + typedef zero_after_free_allocator<Other> other; }; void deallocate(T* p, std::size_t n) -
hebasto commented at 11:32 AM on June 15, 2023: member
Does it make sense to force such changes with the Clang's
-Wreserved-identifierdiagnostic flag?I get a warning about
_EraseTxand this:I got the same. Could be added to this PR, no?
- Brotcrunsher force-pushed on Jun 20, 2023
- Brotcrunsher force-pushed on Jun 20, 2023
- Brotcrunsher force-pushed on Jun 20, 2023
- DrahtBot removed the label Needs rebase on Jun 20, 2023
-
maflcko commented at 7:02 AM on June 20, 2023: member
Please follow the commit subject max len, according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
-
bdea2bb114
scripted-diff: Following the C++ Standard rules for identifiers with _.
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273 -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; } s '__pushKV' 'pushKVEnd' s '_EraseTx' 'EraseTxNoLock' s '_Other' 'Other' -END VERIFY SCRIPT- - Brotcrunsher force-pushed on Jun 20, 2023
-
Brotcrunsher commented at 8:26 AM on June 20, 2023: contributor
Please follow the commit subject max len
Thanks for the feedback, done! One detail question regarding the max length (not important for this Commit, just want to learn for the future). Are CI triggers like "scripted-diff: " considered as part of the subject line and thus part of the 80 chars limitation?
-
maflcko commented at 8:49 AM on June 20, 2023: member
lgtm ACK bdea2bb1147bbd22f8b4fa406262470f9d084215
-
fanquake commented at 10:00 AM on June 21, 2023: member
Does it make sense to force such changes with the Clang's -Wreserved-identifier diagnostic flag?
Doing so is going to require further suppressing, or changes in upstream code. i.e:
./minisketch/include/minisketch.h:2:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier] #define _MINISKETCH_H_ 1 ^ 1 warning generated. ./crypto/ctaes/ctaes.h:8:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier] #define _CTAES_H_ 1 ^ 1 warning generated. ./leveldb/port/thread_annotations.h:16:9: warning: macro name is a reserved identifier [-Wreserved-macro-identifier] #define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) ^ 1 warning generated. minisketch/src/util.h:16:11: warning: macro name is a reserved identifier [-Wreserved-macro-identifier] # define __GNUC_PREREQ(_maj,_min) \ ^ 3 warnings generated. - fanquake merged this on Jun 21, 2023
- fanquake closed this on Jun 21, 2023
- sidhujag referenced this in commit 1fe6a622dd on Jun 21, 2023
- Frank-GER referenced this in commit 44a152da2d on Aug 28, 2023
- Fabcien referenced this in commit dafdf07d97 on Jun 6, 2024
- bitcoin locked this on Jun 20, 2024