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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Thanks for going through all this effort. I think this captures the discussion pretty well. Thanks for including all the citations. If anyone wants to continue on some more points, should we reply here, or reply on the RFC thread?



================
Comment at: llvm/docs/Proposals/VariableNames.rst:86
+[TurnerCamelBack]_.
+Approval for ``lower_case`` was expressed by [CarruthLower]_
+[CarruthCamelBack]_ [TurnerLLDB]_.
----------------
I also prefer this type... you //could// add my name here, but maybe I should ask more generally: it's good that the discussion points in favor/against each style are listed, but as far as individuals that approve/oppose a style, do we plan to run some kind of poll and go with whichever has a majority vote? (Should Chandler/Chris get more votes than me? :) )


================
Comment at: llvm/docs/Proposals/VariableNames.rst:97
+
+Others oppose this idea [HähnleDistinguish]_ [GreeneDistinguish]_
+[HendersonPrefix]_.
----------------
Similarly, I also (slightly) oppose this style, but I'm not sure if you need to add my name to the list, or if we're just going to tally votes at some point


================
Comment at: llvm/docs/Proposals/VariableNames.rst:141
+============================ =============
+DeterministicFiniteAutomaton dfa
+DominatorTree                dt
----------------
An important detail that seems to be left out -- not just here, but also the main llvm style guide -- is scoping of these variables.

For example:
```
for (DeterministicFiniteAutomaton* dfa : allTheDfas) // or all_the_dfas
  dfa->DoSomething();
```
`dfa` is great here, and in fact, a long name might just be distracting.

But, for something like:
```
DeterministicFiniteAutomaton* dfa = GetSomeSpecificDfa();
// ... 200 lines of other stuff ...
dfa->DoSomething(); // I forget, which dfa is this?
```
`dfa` is not a good name -- something that has such a long scope needs a more descriptive name, e.g. //which// dfa we're manipulating.

Which is a long way of saying: I don't think that having a blessed list of acronyms is a good idea. It's going to be entirely dependent on the context. We should just say that acronyms can be considered one word, and is a fine way of using short variable names. (Listing a couple of these as common examples is fine, though).


================
Comment at: llvm/docs/Proposals/VariableNames.rst:297
+
+#. Apply the process to the following projects, with a suitable delay between
+   each to allow gathering further feedback.
----------------
Can we take a stab at defining "suitable delay" here? Would something like 2 weeks be enough? Or maybe a longer time (e.g. 1 month) for the first experiment, shorter times (e.g. 2 weeks) for the next few, and no delay for the remaining ones?


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

https://reviews.llvm.org/D59251