util: Make ArgsManager::GetPathArg more widely usable
ClosedPublic
Actions

Authored by PiRK on Jan 3 2024, 09:53.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC21c074f01f66: util: Make ArgsManager::GetPathArg more widely usable

Summary

util: Add GetPathArg default path argument

Let GetPathArg method be used more places for path arguments that have
default values, like "-settings" and BITCOIN_SETTINGS_FILENAME in the
next commit.

Also:

Fix negated argument handling. Return path{} not path{"0"} when path argument is negated.

Add new tests for default and negated cases

Move GetPathArg() method declaration next to GetArg() declarations. The two methods are close substitutes for each other, so this should help keep them consistent and make them more discoverable.

util, refactor: Use GetPathArg to read "-settings" value

Take advantage of GetPathArg to simplify code slightly.

The change to check-doc is necessary for linting.

Use GetPathArg where possible

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

This is a backport of core#24306

Depends on D15063

Test Plan

ninja all check-all bench-bitcoin

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr24306

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26113
Build 51799: Build Diff	build-without-wallet · lint-circular-dependencies · build-diff · build-debug · build-clang-tidy · build-clang · build-bench
Build 51798: arc lint + arc unit

Event Timeline

PiRK created this revision.Jan 3 2024, 09:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 3 2024, 09:53

PiRK requested review of this revision.Jan 3 2024, 09:53

teamcity edited the summary of this revision. (Show Details)Jan 3 2024, 09:53

punctuation consistency in doxygen comment

PiRK edited the test plan for this revision. (Show Details)Jan 3 2024, 09:57

PiRK added a child revision: D15065: settings: Add update/getPersistent/isIgnored methods.Jan 3 2024, 10:03

Harbormaster completed remote builds in B26113: Diff 43839.Jan 3 2024, 10:17

Harbormaster completed remote builds in B26114: Diff 43840.Jan 3 2024, 10:27

bytesofman added a subscriber: bytesofman.Jan 3 2024, 20:38

bytesofman added inline comments.

src/test/getarg_tests.cpp
258 ↗	(On Diff #43840)	why is this empty here vs `ResetArgs(local_args, "");` in the core PR? similar core has `local_args.` in lower lines instead of `m_local_args`
src/util/system.cpp
396 ↗	(On Diff #43840)	whitespace?
520 ↗	(On Diff #43840)	whitespace?

PiRK added inline comments.Jan 4 2024, 08:18

src/test/getarg_tests.cpp
258 ↗	(On Diff #43840)	We are missing this backport: https://github.com/bitcoin/bitcoin/pull/24375 So our test setup is slightly different, we don't instantiate a local ArgsManager variable in each test, we rely on the LocalTestingSetup defining `m_local_args` and each test case inheriting that class. `ResetArgs` is a member function of the `LocalTestingSetup`, it has access to `m_local_args` defined in the same class and does not need to have it passed as an argument. The end result is the same. From what I understand, core#24735 is a refactoring with no change in behavior.