Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 11, 2022

The file has ~3kLOC and is slow to compile.

Fix both issues by splitting it. (On my machine the compilation goes from 25 seconds previously to 17+10 seconds for the two smaller files)

To review, --color-moved=dimmed-zebra can be used.

@DrahtBot DrahtBot added the Tests label Nov 11, 2022
@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26213 (univalue: Remove confusing getBool/isTrue/isFalse by MarcoFalke)
  • #26028 (More verbose warning for multiple network argument error. by amovfx)
  • #17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
  • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
  • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
  • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

Copy link
Contributor

@shaavan shaavan 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 successfully compiled the branch and ran the test on it.

I think some dependencies would be redundant and can be removed.

in src/test/util_tests.cpp

#include <test/util/logging.h>
#include <test/util/str.h>

and, in src/test/argsman_tests.cpp

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK fa4ec1b

  • The redundant dependencies are removed from the src/test/util_tests.cpp file.

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.

ACK fa4ec1b

// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <util/system.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index d00876bc7..76d64e0d5 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -2,13 +2,13 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <util/system.h>
 #include <fs.h>
 #include <sync.h>
 #include <test/util/logging.h>
 #include <test/util/setup_common.h>
 #include <test/util/str.h>
 #include <util/strencodings.h>
+#include <util/system.h>
 #include <univalue.h>
 
 #include <array>

@RandyMcMillan
Copy link
Contributor

ACK fa4ec1b for:

macOS x84_64:
Darwin ₿ 19.6.0 Darwin Kernel Version 19.6.0: Tue Jun 21 21:18:39 PDT 2022; root:xnu-6153.141.66~1/RELEASE_X86_64 x86_64

macOS Arm64:
Darwin DeepSpaceM1.local 21.6.0 Darwin Kernel Version 21.6.0: Thu Sep 29 20:13:46 PDT 2022; root:xnu-8020.240.7~1/RELEASE_ARM64_T8101 arm64

@maflcko maflcko merged commit 547a963 into bitcoin:master Nov 15, 2022
@maflcko maflcko deleted the 2211-test-split-🌐 branch November 15, 2022 20:50
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants