The new name better describes what the function does.
This function is entirely internal.
Rename AcceptBlock/AcceptBlockHeader to StoreBlock/StoreBlockHeader #7694
pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2016-03-15-naming changing 1 files +11 −11-
pstratem commented at 8:23 PM on March 15, 2016: contributor
-
Rename AcceptBlock and AcceptBlockHeader to StoreBlock and StoreBlockHeader 93e882d7e2
-
laanwj commented at 8:48 AM on March 16, 2016: member
Although this change in isolation makes sense, I don't think renaming these functions is a good idea right now - this is exactly the code that softforks and such change, complicating backporting work. (and in contrast to move-only pulls, renaming is non-trivial to backport over - harder to trace forward/back what function became what)
-
jonasschnelli commented at 12:36 PM on March 16, 2016: contributor
Agree with @laanwj. I also often though about renaming variables and methods, but came always to the same conclusion.
- jonasschnelli added the label Refactoring on Mar 16, 2016
-
jtimon commented at 6:16 PM on March 16, 2016: contributor
On the other hand, this will be "free", whenever we want to change the parameters of those functions. For example, I have proposed to rename CheckBlockHeader and friends to Consensus::CheckBlockHeader several times in the past. but it is free to do such a rename while decoupling the functions from chainparams.o and timedata.o (which are not wanted in libconsensus), see https://github.com/jtimon/bitcoin/commit/46a520ab676fe97a597a55df2f79d694352f8599
-
sipa commented at 11:37 PM on March 16, 2016: member
Agree that things like this can be done simultaneously with refactors/moves.
-
laanwj commented at 11:38 AM on March 17, 2016: member
After a refactor/move it's straightforward to grep where a function went, if there are no renames. If you also rename the function, it's much harder to find out what became of it.
-
jtimon commented at 11:59 AM on March 17, 2016: contributor
What do you mean "where a function went"? It should be called from exactly the same places and its declaration and definition shouldn't move if you are only changing the arguments of a function and its name. Do you think there's anything wrong with renaming the functions in https://github.com/jtimon/bitcoin/commit/46a520ab676fe97a597a55df2f79d694352f8599 ? (Even assuming a more disruptive name change than adding a namespace).
-
laanwj commented at 12:03 PM on March 17, 2016: member
I'm done arguing this, go ahead, if anyone complains while backporting/forward porting softfork code I'll send them to you...
-
jtimon commented at 12:33 PM on March 17, 2016: contributor
I'm just asking sincerely. I never thought about the rename in my example being a concern (since anything conflicting with it is going to conflict in the exact same lines either way), but since it seems there is, I will remove anything related to the consensus namespace in my branch. Sorry for sidetracking the discussion with my example.
-
laanwj commented at 8:15 AM on March 24, 2016: member
Closing this for now. Too much argument for a trivial change. Let's get the softfork stuff done with first.
- laanwj closed this on Mar 24, 2016
- MarcoFalke locked this on Sep 8, 2021
Contributors
Labels