[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:77
+``camelBack`` is consistent with [WebKit]_, [Qt]_ and [Swift]_ while
+``lower_case`` is consistent with [LLDB]_, [Google]_, [Rust]_ and [Python]_.
+
----------------
jhenderson wrote:
> Probably also worth noting that lower_case is consistent with the C++ standard, and maybe other projects like Boost?
I've avoided referencing projects that don't also capitalise class names, as they need to balance different concerns to those we're considering.


================
Comment at: llvm/docs/Proposals/VariableNames.rst:133
+
+The following is a list of acronyms considered sufficiently useful that the
+benefit of using them outweighs the cost of learning them. Acronyms that are
----------------
t.p.northover wrote:
> How was this list derived? It seems a bit skewed towards mid-end development over back-end.
> 
> `MRI` is `MachineRegisterInfo` to me (and about 70% of LLVM code by a quick grep); and `TLI` is `TargetLoweringInfo` (rarer than TargetLibraryInfo this time, but still about 30% of uses).
> 
> I know someone specifically mentioned being surprised by conflicting acronyms when moving to different parts of LLVM, but I think it's rare enough that we should still allow them.
The list was largely compiled from those mentioned in the RFC thread - more can be added in future. I made a mistake with MRI, I've changed it to MachineRegisterInfo.
Was `TargetLoweringInfo` renamed to `TargetLowering`? In that case TLI seems less appropriate. If this is controversial then I'll remove it from the proposal for now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59251