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

[Michael Platings via Phabricator via llvm-commits]

michaelplatings marked an inline comment as done.
michaelplatings added inline comments.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:86
+[TurnerCamelBack]_.
+Approval for ``lower_case`` was expressed by [CarruthLower]_
+[CarruthCamelBack]_ [TurnerLLDB]_.
----------------
rupprecht wrote:
> 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? :) )
Yes, some kind of poll. Exactly how we do this is to be decided. @Charusso has pointed out https://reviews.llvm.org/vote/ but as we in the UK are painfully aware right now, giving people binary choices can lead to no choice at all. I'm inclined to copy Debian's voting method: https://www.debian.org/vote/
How we weight the voting is another interesting question. You could say more contributions = more weight, but given that we're specifically interested in the views of newcomers here that doesn't really work. On the other hand, 1 vote per person would mean that one person could get all their friends to vote for them which is even worse. Potentially we could give 1 vote to any person who has contributed before the discussion started.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:141
+============================ =============
+DeterministicFiniteAutomaton dfa
+DominatorTree                dt
----------------
rupprecht wrote:
> 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).
>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

If an acronym is entirely dependent on the context then I suggest it shouldn't be on the list.

Part of my hope with this list is to be able to say "learn these acronyms and you should be well placed to read most code in LLVM". At the moment the code has too many variables with long scope named using acronyms that may have any number of meanings.

I take your point about names with short scope. If a tool is developed to expand acronyms then it should avoid touching names whose scope is only a few lines.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:294
+      reformat the affected lines according to the rules in ``.clang-format``.
+
+#. Gather feedback and refine the process as appropriate.
----------------
MyDeveloperDay wrote:
> @michaelplatings I have identifier a number of issues
> 
> 
> ```
> bugs.llvm.org/PR41119  - [clang-tidy] readability-identifier-naming incorrectly fixes lambda capture
> bugs.llvm.org/PR41120  - [clang-tidy] readability-identifier-naming incorrectly fixes variables which become keywords
>       (previously call out here with regards to Int being renamed to int)
> bugs.llvm.org/PR41122 - [clang-tidy] readability-identifier-naming misses fixing member variables in destructor
> ```
> 
> (and there could likely me more)
> 
> Which would need to be fixed in clang-tidy prior to any rename activity to prevent this process being painful. I think this is a good dog fooding opportunity for clang-tidy.
Thanks, I've added that to the proposal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59251