scripted-diff: stop using the gArgs wrappers #10607

pull benma wants to merge 2 commits into bitcoin:master from benma:gargs_wrappers changing 40 files +348 −394

benma commented at 8:58 PM on June 15, 2017: none

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

The first commit removes the uses, the second commit removes the wrappers themselves.
fanquake added the label Refactoring on Jun 16, 2017
practicalswift commented at 4:25 AM on June 16, 2017: contributor

utACK 39faab76321bc6bf217ffdc9b380c60b9d99416f
sipa commented at 9:52 PM on June 16, 2017: member

utACK 39faab76321bc6bf217ffdc9b380c60b9d99416f
fanquake commented at 12:43 AM on June 28, 2017: member

Concept ACK. Needs rebase.
jnewbery commented at 8:23 PM on July 10, 2017: member

Tested ACK 39faab76321bc6bf217ffdc9b380c60b9d99416f
benma force-pushed on Jul 13, 2017
benma commented at 9:43 AM on July 13, 2017: none

Rebased.
TheBlueMatt commented at 7:23 PM on July 14, 2017: member

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 commented at 7:30 PM on July 14, 2017: none

@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 commented at 7:45 PM on July 14, 2017: member

Hmm, I figured it'd be easier to just sed/gArgs/locallyReferencedArgs/ in files, but I guess it doesnt matter much either way.
benma commented at 8:28 PM on July 14, 2017: none

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 commented at 12:53 PM on July 17, 2017: contributor

Needs rebase :-)
benma force-pushed on Jul 17, 2017
benma commented at 9:45 PM on July 17, 2017: none

Rebased 😆
benma force-pushed on Jul 22, 2017
benma commented at 7:48 AM on July 22, 2017: none

Rebased. @sipa I'd be glad if this could be merged soon, as it conflicts with other changes very often.
benma commented at 6:34 PM on July 23, 2017: none

How can I retrigger CI?
sipa commented at 6:35 PM on July 23, 2017: member

@benma I've restarted Travis.
laanwj added this to the milestone 0.15.0 on Jul 28, 2017
TheBlueMatt commented at 6:36 PM on August 1, 2017: member

Needs rebase.

scripted-diff: stop using the gArgs wrappers

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-

bb3d105d76

remove unused gArgs wrappers 3b22edd106
benma force-pushed on Aug 1, 2017
benma commented at 7:31 PM on August 1, 2017: none

@TheBlueMatt thx. Rebased.
promag commented at 1:02 PM on August 2, 2017: member

I believe 3b22edd has an unrelated change that should be in bb3d105, otherwise tested ACK 3b22edd.
benma commented at 1:38 PM on August 2, 2017: none

@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 commented at 3:09 PM on August 2, 2017: member

@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 commented at 9:49 PM on August 2, 2017: none

@promag, I prefer it if each is compiles and passes tests, and also think it would be more confusing and unneeded work.
MarcoFalke commented at 12:33 PM on August 3, 2017: member

@benma is right. Commits should be atomic (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#contributor-workflow)
ryanofsky commented at 3:24 PM on August 3, 2017: member

Slightly tested ACK 3b22edd106f51cc25b200e41ffaab10877c50144. Confirmed both commits compile, so I think the atomic commit issue is resolved.
MarcoFalke commented at 4:12 PM on August 3, 2017: member

utACK 3b22edd106f51cc25b200e41ffaab10877c50144
promag commented at 7:45 AM on August 6, 2017: member

ACK 3b22edd.
promag commented at 2:03 PM on August 6, 2017: member

@laanwj it would be nice to have this in so #10976 can be rebased.
sipa commented at 10:07 PM on August 6, 2017: member

Let's try to merge this right before branching off.
MarcoFalke commented at 1:02 PM on August 8, 2017: member

utACK 3b22edd106f51cc25b200e41ffaab10877c50144, but it conflicts with #10882, which is tagged for 0.15.
benma commented at 8:27 AM on August 9, 2017: none

I don't mind rebasing once more after #10882 is merged.
MarcoFalke commented at 2:25 PM on August 14, 2017: member

Needs rebase.
laanwj commented at 3:21 PM on August 14, 2017: member

Rebased and merged via c2704ec
laanwj closed this on Aug 14, 2017
laanwj referenced this in commit c2704ec98a on Aug 14, 2017
jtimon commented at 9:36 PM on August 14, 2017: contributor

Oh, nice, I was planning to do this but thank you for doing it faster!
PastaPastaPasta referenced this in commit 6bfbe6053d on Jun 24, 2019
PastaPastaPasta referenced this in commit 5d5171f478 on Jun 24, 2019
barrystyle referenced this in commit e6b8c46d88 on Jan 22, 2020
barrystyle referenced this in commit ade2fe8f16 on Jan 22, 2020
furszy referenced this in commit de5b240881 on Oct 28, 2020
DrahtBot locked this on Sep 8, 2021