Skip to content

Conversation

@fanquake
Copy link
Member

Refactor and introduce modernize-use-using into our clang-tidy job. Given we already use both, consolidate to the later.

See: https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-use-using.html.

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

I see you refactored typedefs in *.cpp files, can it be done in header files as well?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
  • #26158 (bench: add "priority level" to the benchmark framework by furszy)
  • #25620 (wallet: Introduce AddressBookManager by furszy)
  • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
  • #21590 (Safegcd-based modular inverses in MuHash3072 by sipa)
  • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
  • #16195 (util: Use void* throughout support/lockedpool.h by jkczyz)

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.

@fanquake
Copy link
Member Author

fanquake commented Aug 1, 2022

I see you refactored typedefs in *.cpp files, can it be done in header files as well?

Added some changes to header files.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK d1885b434f3612ac1efe926e230a440bd7eed366

@fanquake
Copy link
Member Author

Rebased for #25707.

@fanquake fanquake requested a review from aureleoules August 19, 2022 15:34
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can change these as well.

Git diff
diff --git a/src/blockencodings.h b/src/blockencodings.h
index 67c4e5715..700ebb252 100644
--- a/src/blockencodings.h
+++ b/src/blockencodings.h
@@ -74,14 +74,13 @@ struct PrefilledTransaction {
     SERIALIZE_METHODS(PrefilledTransaction, obj) { READWRITE(COMPACTSIZE(obj.index), Using<TransactionCompression>(obj.tx)); }
 };
 
-typedef enum ReadStatus_t
-{
+using ReadStatus = enum ReadStatus_t {
     READ_STATUS_OK,
     READ_STATUS_INVALID, // Invalid object, peer is sending bogus crap
     READ_STATUS_FAILED, // Failed to process object
     READ_STATUS_CHECKBLOCK_FAILED, // Used only by FillBlock to indicate a
                                    // failure in CheckBlock.
-} ReadStatus;
+};
 
 class CBlockHeaderAndShortTxIDs {
 private:
diff --git a/src/protocol.h b/src/protocol.h
index b85dc0d82..af105f90a 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -33,7 +33,7 @@ public:
     static constexpr size_t MESSAGE_SIZE_OFFSET = MESSAGE_START_SIZE + COMMAND_SIZE;
     static constexpr size_t CHECKSUM_OFFSET = MESSAGE_SIZE_OFFSET + MESSAGE_SIZE_SIZE;
     static constexpr size_t HEADER_SIZE = MESSAGE_START_SIZE + COMMAND_SIZE + MESSAGE_SIZE_SIZE + CHECKSUM_SIZE;
-    typedef unsigned char MessageStartChars[MESSAGE_START_SIZE];
+    using MessageStartChars = unsigned char[MESSAGE_START_SIZE];
 
     explicit CMessageHeader() = default;
 
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 01e855605..31fd72ce3 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -84,7 +84,7 @@ void RPCUnsetTimerInterface(RPCTimerInterface *iface);
  */
 void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nSeconds);
 
-typedef RPCHelpMan (*RpcMethodFnType)();
+using RpcMethodFnType = RPCHelpMan (*)();
 
 class CRPCCommand
 {
diff --git a/src/script/bitcoinconsensus.h b/src/script/bitcoinconsensus.h
index 8fea42e4b..fda5e3c5e 100644
--- a/src/script/bitcoinconsensus.h
+++ b/src/script/bitcoinconsensus.h
@@ -33,7 +33,7 @@ extern "C" {
 
 #define BITCOINCONSENSUS_API_VER 1
 
-typedef enum bitcoinconsensus_error_t
+using bitcoinconsensus_error = enum bitcoinconsensus_error_t
 {
     bitcoinconsensus_ERR_OK = 0,
     bitcoinconsensus_ERR_TX_INDEX,
@@ -41,7 +41,8 @@ typedef enum bitcoinconsensus_error_t
     bitcoinconsensus_ERR_TX_DESERIALIZE,
     bitcoinconsensus_ERR_AMOUNT_REQUIRED,
     bitcoinconsensus_ERR_INVALID_FLAGS,
-} bitcoinconsensus_error;
+};
 
 /** Script verification flags */
 enum
diff --git a/src/script/script_error.h b/src/script/script_error.h
index 44e68fe0f..2674ca8dd 100644
--- a/src/script/script_error.h
+++ b/src/script/script_error.h
@@ -8,7 +8,7 @@
 
 #include <string>
 
-typedef enum ScriptError_t
+using ScriptError = enum ScriptError_t
 {
     SCRIPT_ERR_OK = 0,
     SCRIPT_ERR_UNKNOWN_ERROR,
@@ -83,7 +83,7 @@ typedef enum ScriptError_t
     SCRIPT_ERR_SIG_FINDANDDELETE,
 
     SCRIPT_ERR_ERROR_COUNT
-} ScriptError;
+};
 
 #define SCRIPT_ERR_LAST SCRIPT_ERR_ERROR_COUNT
 
diff --git a/src/support/events.h b/src/support/events.h
index 0a74d0236..24f807ae3 100644
--- a/src/support/events.h
+++ b/src/support/events.h
@@ -19,7 +19,7 @@ struct type##_deleter {\
     }\
 };\
 /* unique ptr typedef */\
-typedef std::unique_ptr<struct type, type##_deleter> raii_##type
+using raii_##type = std::unique_ptr<struct type, type##_deleter>;
 
 MAKE_RAII(event_base);
 MAKE_RAII(event);
diff --git a/src/support/lockedpool.h b/src/support/lockedpool.h
index 1c0d0669b..bbbb5d374 100644
--- a/src/support/lockedpool.h
+++ b/src/support/lockedpool.h
@@ -139,7 +139,8 @@ public:
 
     /** Callback when allocation succeeds but locking fails.
      */
-    typedef bool (*LockingFailed_Callback)();
+    using LockingFailed_Callback = bool (*)();
 
     /** Memory statistics. */
     struct Stats
diff --git a/src/tinyformat.h b/src/tinyformat.h
index bedaa1400..e938ea046 100644
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -221,7 +221,9 @@ struct is_convertible
 
 
 // Detect when a type is not a wchar_t string
-template<typename T> struct is_wchar { typedef int tinyformat_wchar_is_not_supported; };
+template<typename T> struct is_wchar { 
+    using tinyformat_wchar_is_not_supported = int;
+};
 template<> struct is_wchar<wchar_t*> {};
 template<> struct is_wchar<const wchar_t*> {};
 template<int n> struct is_wchar<const wchar_t[n]> {};
@@ -332,7 +334,7 @@ inline void formatValue(std::ostream& out, const char* /*fmtBegin*/,
 #ifndef TINYFORMAT_ALLOW_WCHAR_STRINGS
     // Since we don't support printing of wchar_t using "%ls", make it fail at
     // compile time in preference to printing as a void* at runtime.
-    typedef typename detail::is_wchar<T>::tinyformat_wchar_is_not_supported DummyType;
+    using DummyType = typename detail::is_wchar<T>::tinyformat_wchar_is_not_supported;
     (void) DummyType(); // avoid unused type warning with gcc-4.8
 #endif
     // The mess here is to support the %c and %p conversions: if these
@@ -958,7 +960,7 @@ class FormatList
 };
 
 /// Reference to type-opaque format list for passing to vformat()
-typedef const FormatList& FormatListRef;
+using FormatListRef = const FormatList&;
 
 
 namespace detail {

@fanquake
Copy link
Member Author

I believe you can change these as well.

Thanks. Incorporated those are that aren't upstream code, i.e tinyformat.

@fanquake fanquake force-pushed the tidy_modernize_use_using branch from 90b52be to 313241d Compare August 20, 2022 15:03
@aureleoules
Copy link
Contributor

ACK 313241d757c022d978d87e1abd9ab4bb68d45986

@maflcko
Copy link
Member

maflcko commented Aug 24, 2022

What is the point of this other than causing merge conflicts, given that the linter doesn't even enforce any of this in header files?

I mean it is great that we have clang-tidy but we should use it for transformations/checks that prevent bugs, not as an excuse for one-off (+ endless follow-up) style cleanups. Especially if the new rule doesn't even enforce the new style.

@ryanofsky
Copy link
Contributor

Makes sense that if you're going to to use clang-tidy for a cleanup, you should probably look into it enabling it as a lint-check too so the cleanup does not need to be repeated. I don't have so much of a problem with one-off cleanups or repeated cleanups, though.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional code review ACK 313241d757c022d978d87e1abd9ab4bb68d45986 if it is ok to use c++ syntax in the bitcoinconsensus.h file

@fanquake fanquake force-pushed the tidy_modernize_use_using branch from 313241d to 6edd971 Compare August 31, 2022 10:36
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 6edd971. Only changes since last review were suggested cleanups

#include <string>

typedef enum ScriptError_t
using ScriptError = enum ScriptError_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "refactor: use "using" over "typedef"" (dfd9115)

Would be good for a followup (possible future PR) to drop ScriptError_t in tests and just change this to enum ScriptError

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake fanquake closed this Dec 5, 2022
@fanquake fanquake deleted the tidy_modernize_use_using branch November 6, 2023 10:28
@bitcoin bitcoin locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants