[PATCH] D59251: [Documentation] Proposal for plan to change variable names

[Michael Platings via Phabricator via llvm-commits]

michaelplatings marked 9 inline comments as done.
michaelplatings added inline comments.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:90
+
+Differentiating variable kinds
+------------------------------
----------------
lattner wrote:
> FWIW, I personally consider this to be a totally orthogonal discussion to the other issues, I think it would be nice to separate it out just so we have some hope of converging an already contentious topic.  Ratcheting forward one decision at a time seems like better way to make progress.
I agree that it's orthogonal to reducing the number of acronyms (my personal intention with all this) but there seems to be a strong sentiment which we can't ignore that differentiating member variables should be considered as part of changing naming policy. For that reason I'm including it at this stage. I hope that results gathered from the experimentation phase will inform the discussion and we can agree at that point on a suitable way forward, which may be to defer this particular change as you suggest.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:144
+DominatorTree                dt
+Function                     f
+LoopInfo                     li
----------------
lattner wrote:
> mehdi_amini wrote:
> > side note: I feel that single letter variable name are annoying (can't easily search in a text editor for example), I would rather use a short name like `func`.
> Yeah, maybe this could include a list, like f, fn, func, etc.  There is a diversity of contractions used here.  Standardizing this is a theoretically nice thing, but a distraction from the core issue in this discussion and not particularly important in the big scheme of things (unlikely to cause confusion).
I was under the misconception that `F` was one of the acronyms that people were fond of. I've removed it for now.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:220
+
+Big bang
+--------
----------------
ruiu wrote:
> By the way, do you have anything in mind about what tool can be used for batch renaming? Do we have to hack it up based on clang-format or something?
Primarily `clang-tidy`. There will be some extra work to resolve name conflicts (there are some variables named `Int` for example) but we'll flush out such issues during the experimentation phase (currently step 3 in the provisional plan).


================
Comment at: llvm/docs/Proposals/VariableNames.rst:254
+
+Minimising cost of downstream merges
+************************************
----------------
lattner wrote:
> FWIW, It has never been a goal for LLVM To prevent downstream merge conflicts.  The "party line" (painful as it may be in practice) is that mainline moves ahead full speed without worrying about this, and anyone adversely affected should work to get their changes merged to mainline.
> 
> This isn't likely to affect the public APIs in llvm/include in a significant way, so it really only affects people with diffs against core code.
Yes, and I'm fully supportive of that policy. But if we can make downstream folks' lives easier without compromising our choices upstream then I'm happy to put in the effort to do that.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:264
+resolving all conflicts by choosing their own version. This could be tested on
+the [SVE]_ fork.
+
----------------
mehdi_amini wrote:
> This can be seen as an advantage for a mass rename: it is a "one-time cost" (that can be helped by clang-tidy), while a progressive renaming will lead to many spurious merge conflicts (and successful merge breaking the builds, or worse changing the runtime behavior!!) for downstream users for multiple years.
> It seems be easier to deal with a one time merge that is NFC rather than having many semantic change patches along the years that create conflict on variable naming.
True, although see Chris Lattner's comment about the "party line" above - on that basis it's not something that should sway our decision.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59251/new/

https://reviews.llvm.org/D59251