Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renamed UniValue::__pushKV to UniValue::pushKVEnd. #27822

Merged
merged 1 commit into from Jun 21, 2023

Conversation

Brotcrunsher
Copy link
Contributor

Any identifier starting with 2 _ is reserved for the compiler and thus must not be used.

See: https://stackoverflow.com/a/228797/7130273

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 4, 2023

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

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.

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
Copy link
Member

hebasto commented Jun 4, 2023

See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs for changes like this.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2023

lgtm ACK acad989

@hebasto
Copy link
Member

hebasto commented Jun 5, 2023

Does it make sense to force such changes with the Clang's -Wreserved-identifier diagnostic flag?

@fanquake
Copy link
Member

@Brotcrunsher are you going to rebase this?

@maflcko
Copy link
Member

maflcko commented Jun 15, 2023

Does it make sense to force such changes with the Clang's -Wreserved-identifier diagnostic flag?

I get a warning about _EraseTx and 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
Copy link
Member

hebasto commented Jun 15, 2023

Does it make sense to force such changes with the Clang's -Wreserved-identifier diagnostic flag?

I get a warning about _EraseTx and this:

I got the same. Could be added to this PR, no?

@maflcko
Copy link
Member

maflcko commented Jun 20, 2023

Please follow the commit subject max len, according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

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
Copy link
Contributor Author

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
Copy link
Member

maflcko commented Jun 20, 2023

lgtm ACK bdea2bb

@fanquake
Copy link
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 fanquake merged commit e410fb7 into bitcoin:master Jun 21, 2023
14 of 15 checks passed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 21, 2023
bdea2bb scripted-diff: Following the C++ Standard rules for identifiers with _. (Brotcrunsher)

Pull request description:

  Any identifier starting with 2 _ is reserved for the compiler and thus must not be used.

  See: https://stackoverflow.com/a/228797/7130273

ACKs for top commit:
  MarcoFalke:
    lgtm ACK bdea2bb

Tree-SHA512: 74c8e676449f3f61476d846bfd2c514103c8914e13c4a0db841203abdc0267c25ddc6ed57d6791459efe3edea17753a1b53c3795071ddfe8aba8662521063407
Frank-GER pushed a commit to Frank-GER/syscoin that referenced this pull request Aug 28, 2023
'__pushKV' -> 'pushKVEnd'
see btc PR bitcoin#27822
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants