Skip to content

scripted-diff: stop using the gArgs wrappers #10607

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

scripted-diff: stop using the gArgs wrappers #10607

wants to merge 2 commits into from

Conversation

benma
Copy link

@benma benma commented Jun 15, 2017

Following #9494, where they were introduced to ease the transition.

The first commit removes the uses, the second commit removes the wrappers themselves.

@practicalswift
Copy link
Contributor

utACK 39faab76321bc6bf217ffdc9b380c60b9d99416f

1 similar comment
@sipa
Copy link
Member

sipa commented Jun 16, 2017

utACK 39faab76321bc6bf217ffdc9b380c60b9d99416f

@fanquake
Copy link
Member

Concept ACK. Needs rebase.

@jnewbery
Copy link
Contributor

Tested ACK 39faab76321bc6bf217ffdc9b380c60b9d99416f

@benma
Copy link
Author

benma commented Jul 13, 2017

Rebased.

@TheBlueMatt
Copy link
Contributor

Wait, is this the direction we want to go in, or the opposite? eg i figured we'd want to slowly move towards storing references to gArgs in things that need them, eg as we move towards classes which contain things (CConnman, CChainState, etc) we'd either pass in the options relevant to those objects (ala CConnman's constructor) or pass them the gArgs to avoid the global.

@benma
Copy link
Author

benma commented Jul 14, 2017

@TheBlueMatt what you mention makes sense to me, but it seems unrelated to this PR, which is just about removing the wrappers (which use the same global). This PR is also good prep-work for storing references to gArgs in classes in that sense.

@TheBlueMatt
Copy link
Contributor

Hmm, I figured it'd be easier to just sed/gArgs/locallyReferencedArgs/ in files, but I guess it doesnt matter much either way.

@benma
Copy link
Author

benma commented Jul 14, 2017 •
edited
Loading

One can easily do this after this PR, whereas before, gArgs didn't even appear in all the places you'd expect it to and you'd have to use the same kind of complex regexp to catch them all as in the scripted diff script. I agree it doesn't matter much, but it does make it easier I think.

It could be a long time too until someone starts doing this work, and in the meantime, we can avoid the confusion of how to use it, saving review cycles/time.

@practicalswift
Copy link
Contributor

Needs rebase :-)

@benma
Copy link
Author

benma commented Jul 17, 2017

Rebased 😆

@benma
Copy link
Author

benma commented Jul 22, 2017

Rebased.

@sipa I'd be glad if this could be merged soon, as it conflicts with other changes very often.

@benma
Copy link
Author

benma commented Jul 23, 2017

How can I retrigger CI?

@sipa
Copy link
Member

sipa commented Jul 23, 2017

@benma I've restarted Travis.

@laanwj laanwj added this to the 0.15.0 milestone Jul 28, 2017
@TheBlueMatt
Copy link
Contributor

Needs rebase.

Marko Bencun added 2 commits August 1, 2017 21:17
scripted-diff: stop using the gArgs wrappers
bb3d105 
They were temporary additions to ease the transition.

-BEGIN VERIFY SCRIPT-
find src/ -name "*.cpp" ! -wholename "src/util.h" ! -wholename "src/util.cpp" | xargs perl -i -pe 's/(?<!\.)(ParseParameters|ReadConfigFile|IsArgSet|(Soft|Force)?(Get|Set)(|Bool|)Arg(s)?)\(/gArgs.\1(/g'
-END VERIFY SCRIPT-
remove unused gArgs wrappers
3b22edd
@benma
Copy link
Author

benma commented Aug 1, 2017

@TheBlueMatt thx. Rebased.

@promag
Copy link
Contributor

promag commented Aug 2, 2017

I believe 3b22edd has an unrelated change that should be in bb3d105, otherwise tested ACK 3b22edd.

@benma
Copy link
Author

benma commented Aug 2, 2017

@promag thank you. util.h and util.cpp are excluded from the scripted diff due to false positives. Making it work would take an unreasonable effort, so I did those few instances manually.

@promag
Copy link
Contributor

promag commented Aug 2, 2017

@benma what about switching commit order? I know it's not the logical thing to do in terms of commit order, but in terms of what the PR stands for, IMO it's fine.

@benma
Copy link
Author

benma commented Aug 2, 2017

@promag, I prefer it if each is compiles and passes tests, and also think it would be more confusing and unneeded work.

@maflcko
Copy link
Member

maflcko commented Aug 3, 2017

@benma is right. Commits should be atomic (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#contributor-workflow)

@promag promag mentioned this pull request Aug 3, 2017
Merged
ryanofsky
ryanofsky reviewed Aug 3, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Slightly tested ACK 3b22edd. Confirmed both commits compile, so I think the atomic commit issue is resolved.

@maflcko
Copy link
Member

maflcko commented Aug 3, 2017

utACK 3b22edd

promag
promag reviewed Aug 6, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

ACK 3b22edd.

@promag
Copy link
Contributor

promag commented Aug 6, 2017

@laanwj it would be nice to have this in so #10976 can be rebased.

@sipa
Copy link
Member

sipa commented Aug 6, 2017

Let's try to merge this right before branching off.

@maflcko
Copy link
Member

maflcko commented Aug 8, 2017

utACK 3b22edd, but it conflicts with #10882, which is tagged for 0.15.

@benma
Copy link
Author

benma commented Aug 9, 2017

I don't mind rebasing once more after #10882 is merged.

@maflcko
Copy link
Member

maflcko commented Aug 14, 2017

Needs rebase.

@laanwj
Copy link
Member

laanwj commented Aug 14, 2017

Rebased and merged via c2704ec

@laanwj laanwj closed this Aug 14, 2017
laanwj added a commit that referenced this pull request Aug 14, 2017
@laanwj
Merge #10607: scripted-diff: stop using the gArgs wrappers
c2704ec 
fcbde90 remove unused gArgs wrappers (Marko Bencun)
bb81e17 scripted-diff: stop using the gArgs wrappers (Marko Bencun)

Tree-SHA512: 49190740dfc723d680a093b625bf4e4e010bc121741b035d790f787e73eedd74966e4ceaf56975050d06b5d7d6cd8f46f9deb5cc78569df1e9faf0d7e543ee21
@jnewbery jnewbery mentioned this pull request Aug 14, 2017
Merged
@jtimon
Copy link
Contributor

jtimon commented Aug 14, 2017

Oh, nice, I was planning to do this but thank you for doing it faster!

This was referenced Mar 27, 2018
Merged
Open
@codablock codablock mentioned this pull request Jun 24, 2019
Merged
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 24, 2019
@PastaPastaPasta
Scripted diff `find src/ -name "*.cpp" ! -wholename "src/util.h" ! -w…
6bfbe60 
…holename "src/util.cpp" | xargs perl -i -pe 's/(?<!\.)(ParseParameters|ReadConfigFile|IsArgSet|(Soft|Force)?(Get|Set)(|Bool|)Arg(s)?)\(/gArgs.\1(/g'` based on bb81e17

(bitcoin#10607)

Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 24, 2019
@PastaPastaPasta
remove unused gArgs wrappers
5d5171f 
(bitcoin#10607)
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@PastaPastaPasta
Scripted diff `find src/ -name "*.cpp" ! -wholename "src/util.h" ! -w…
e6b8c46 
…holename "src/util.cpp" | xargs perl -i -pe 's/(?<!\.)(ParseParameters|ReadConfigFile|IsArgSet|(Soft|Force)?(Get|Set)(|Bool|)Arg(s)?)\(/gArgs.\1(/g'` based on bb81e17

(bitcoin#10607)

Signed-off-by: Pasta <pasta@dashboost.org>
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
remove unused gArgs wrappers
ade2fe8 
(bitcoin#10607)
@Fuzzbawls Fuzzbawls mentioned this pull request Oct 23, 2020
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Oct 28, 2020
@furszy
Merge #1938: [Core] Arg parsing overhaul
de5b240 
4709ff8 remove now unused gArgs wrappers (Fuzzbawls)
cc40f8d Manual prep for gArg wrapper removal (Fuzzbawls)
b3c843c scripted-diff: stop using the gArgs wrappers (Fuzzbawls)
5ce3844 Migrate g_logger fields to gArgs (Fuzzbawls)
72a6f56 Track negated arguments in the argument paser. (Fuzzbawls)
3142e1b Add additional tests for GetBoolArg() (Fuzzbawls)
4aef2f3 Fix constness of ArgsManager methods (Fuzzbawls)
13fd27d Fix indentation after 'Remove redundant calls to gArgs.IsArgSet()' (Fuzzbawls)
a4114f3 Util: Remove redundant calls to gArgs.IsArgSet() (Fuzzbawls)
92e7227 Util: Small improvements in gArgs usage (Fuzzbawls)
a8b26e7 Util: Put mapMultiArgs inside ArgsManager (Fuzzbawls)
d2f2e25 scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs (Fuzzbawls)
b2abc59 Util: Create ArgsManager class... (Fuzzbawls)
8c4d9c5 Add a ForceSetArg method for testing (Fuzzbawls)
59d6529 Lock mapArgs/mapMultiArgs access in util (Fuzzbawls)
f7f2689 Un-expose mapArgs from util.h (Fuzzbawls)
7894b07 Get rid of mapArgs direct access in ZMQ construction (Matt Corallo)
b28789f Introduce (and use) an IsArgSet accessor method (Fuzzbawls)
4d8ecf9 Fix non-const mapMultiArgs[] access after init. (Fuzzbawls)
391388d Remove arguments to ReadConfigFile (Fuzzbawls)

Pull request description:

  This is a cumulation of several upstream backports surrounding the argument parsing code.

  Included here are the following upstream PRs:
  - bitcoin#9243
  - bitcoin#9494
  - bitcoin#10118
  - bitcoin#10901
  - bitcoin#12713
  - bitcoin#10607

  I've also refactored the global logging argument parsing to match, as that was an "out-of-order" PR that was previously backported to match our previous state.

ACKs for top commit:
  random-zebra:
    utACK 4709ff8
  furszy:
    cool encapsulation, utACK 4709ff8 and merging

Tree-SHA512: 74c8dc1b2280b726eff44855e9908f3563d4ef6493f296ebfafe240ac6470bc45cbc75baac614ed59dfda9e1377b7da7d47a695dfc05e364cada2468bb99a258
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.