[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I would also love to see this conceptually, but think it will be pretty 
polarizing in the community.  It is worth another RFC thread before investing 
much time in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

Hi Sam, I won't be able to take this forward but you have my encouragement. To 
facilitate this change I got as far as changing Git [1], and GitHub has been 
updated accordingly [2], but I ran out of steam before getting to the change 
itself.
I'd be happy to let someone else (you?) take the lead and commandeer the change.

[1] https://moxio.com/blog/ignoring-bulk-change-commits-with-git-blame/
[2] 
https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.
Herald added a project: All.

Is there still appetite to land this change?We made the switch over in LLD 
a while back without any issues that I know of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D57896#1435245 , @lattner wrote:

> FWIW, my suggestion is *not* to expand names like DRE to decl_ref_expr, I 
> agree that doesn't add clarity to the code.  Two possibilities: "dre", or 
> "decl" which is what I would write today.


I totally agree with you, wherever I can I write that out for clarification. 
Please note that in my example there is two `Expr` and that is why I pointed 
out we need acronyms so we cannot really use `expr` and acronyms usually 
capital, that is why we went back to the default CamelCase standard. It was a 
little brainstorming and ping for you guys because I believe you would put 
those polls out and create a better code-base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

In D57896#1434877 , @Charusso wrote:

> static Optional
>  getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
>  //...
>
>   if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) {
> if (const auto *VD = dyn_cast_or_null(DRE->getDecl())) {
>
> //...
>  }
>
>   would be:
>  
>
>
> static Optional 
>   |
>  getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) 
> {  |
>  //...
>|
>
>   if (const auto *decl_ref_expr = 
> dyn_cast_or_null(cond_var_expr)) {
> if (const auto *var_decl = 
> dyn_cast_or_null(decl_ref_expr->getDecl())) {
>
> //... 
>   |
>  } whoops 
> column-81 ~^
>
>   Hungarian notation on members and globals are cool idea. However, the 
> notation is made without the `_` part, so I think `mMember` is better than 
> `m_member` as we used to 80-column standard and it is waste of space and 
> hurts your C-developer eyes. I would recommend `b` prefix to booleans as 
> Unreal Engine 4 styling is used to do that (`bIsCoolStyle`) and it is handy. 
> It is useful because booleans usually has multiple prefixes: `has, have, is` 
> and you would list all the booleans together in autocompletion. Yes, there is 
> a problem: if the notation is not capital like the pure Hungarian notation 
> then it is problematic to list and we are back to the `BIsCapitalLetter` and 
> `MMember` CamelCase-world where we started (except one project). I think 
> @lattner could say if it is useful as all the Apple projects based on those 
> notations and could be annoying.


FWIW, my suggestion is *not* to expand names like DRE to decl_ref_expr, I agree 
that doesn't add clarity to the code.  Two possibilities: "dre", or "decl" 
which is what I would write today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added subscribers: chandlerc, Charusso.
Charusso added a comment.

In D57896#1406812 , @lattner wrote:

> I can understand Zach's position here, but LLDB has historically never 
> conformed to the general LLVM naming or other conventions due to its 
> heritage.  It should not be taken as precedent that the rest of the project 
> should follow.
>
> In any case, I think that it is best for this discussion to take place on the 
> llvm-dev list where it is likely to get the most visibility.  Would you mind 
> moving comments and discussions there?


Hey! Random Clang developer is here after this topic became a little-bit dead 
as not everyone subbed to LLVM dev-list. I think the best solution for every 
difficult question is to let the users decide their own future among all the 
projects. I would announce polls (https://reviews.llvm.org/vote/) and announce 
them on every dev-list.

I do not see any better solution to decide if we would code like `DRE`, `VD` 
versus `expr`, `decl` as @lattner would code. And I am not sure if everyone 
happy with `this_new_styling` as @chandlerc and @zturner would code. E.g. I am 
not happy because I know my if-statements would take two lines of code instead 
of one and it would be super-duper-mega ugly and difficult to read. Here is an 
example:

  static Optional
  getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
  //...
if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) {
  if (const auto *VD = dyn_cast_or_null(DRE->getDecl())) {
  //...
  }

would be:

  static Optional 
  |
  getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) 
{  |
  //... 
  |
if (const auto *decl_ref_expr = 
dyn_cast_or_null(cond_var_expr)) {
  if (const auto *var_decl = 
dyn_cast_or_null(decl_ref_expr->getDecl())) {
  //... 
  |
  } whoops 
column-81 ~^

Hungarian notation on members and globals are cool idea. However, the notation 
is made without the `_` part, so I think `mMember` is better than `m_member` as 
we used to 80-column standard and it is waste of space and hurts your 
C-developer eyes. I would recommend `b` prefix to booleans as Unreal Engine 4 
styling is used to do that (`bIsCoolStyle`) and it is handy. It is useful 
because booleans usually has multiple prefixes: `has, have, is` and you would 
list all the booleans together in autocompletion. Yes, there is a problem: if 
the notation is not capital like the pure Hungarian notation then it is 
problematic to list and we are back to the `BIsCapitalLetter` and `MMember` 
CamelCase-world where we started (except one project). I think @lattner could 
say if it is useful as all the Apple projects based on those notations and 
could be annoying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I can understand Zach's position here, but LLDB has historically never 
conformed to the general LLVM naming or other conventions due to its heritage.  
It should not be taken as precedent that the rest of the project should follow.

In any case, I think that it is best for this discussion to take place on the 
llvm-dev list where it is likely to get the most visibility.  Would you mind 
moving comments and discussions there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406412 , @MyDeveloperDay 
wrote:

> In D57896#1406407 , @zturner wrote:
>
> > If I read the post correctly, it was actually agreeing with me (because it 
> > said "to reinforce your point...".  Meaning that something such as 
> > `lowerCaseCamel` would be the third style being referred to
>
>
> Correct! just acknowledging your point from a different perspective.


Doh!  Sorry for the noise.

It looks like the RFC thread has mostly turned into a transition-plan debate, 
so should we work on the actual convention description here? Extracting the 
naming conventions from the 3.6-era link mentioned above, we have:

- types and classes are UpperCamelCase (this is unchanged from current LLVM 
style)
- methods are UpperCamelCase (this is also the old LLVM style IIRC)
- variables are snake_case
- static data members add "g_" prefix to variable style (although I see a 
proposal for "s_" instead)
- nonstatic data members add "m_" prefix to variable style

Did I miss anything really important?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57896#1406407 , @zturner wrote:

> If I read the post correctly, it was actually agreeing with me (because it 
> said "to reinforce your point...".  Meaning that something such as 
> `lowerCaseCamel` would be the third style being referred to


Correct! just acknowledging your point from a different perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1406401 , @probinson wrote:

> In D57896#1406353 , @MyDeveloperDay 
> wrote:
>
> > I can't argue with anything you say.. but I guess to reinforce your point 
> > introducing what is effectively a 3rd style would likely cause even more 
> > jarring...
>
>
> Zach isn't introducing a new style, the style already exists and is 
> consistently used by what I think is our 3rd largest subproject.  It happens 
> not to be used at all by the two largest subprojects, but those subprojects 
> already aren't consistent with themselves.
>  I would not mind a more concerted effort to migrate to whatever style we 
> pick, which was notably lacking last time around. Then the jarring 
> inconsistencies would go away, and we could all get back to complaining about 
> content and not style.


If I read the post correctly, it was actually agreeing with me (because it said 
"to reinforce your point...".  Meaning that something such as `lowerCaseCamel` 
would be the third style being referred to, while my proposal keeps the number 
of styles to 2.  But, maybe I read it wrong.  If I read it right, then 
obviously I agree :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D57896#1406353 , @MyDeveloperDay 
wrote:

> I can't argue with anything you say.. but I guess to reinforce your point 
> introducing what is effectively a 3rd style would likely cause even more 
> jarring...


Zach isn't introducing a new style, the style already exists and is 
consistently used by what I think is our 3rd largest subproject.  It happens 
not to be used at all by the two largest subprojects, but those subprojects 
already aren't consistent with themselves.
I would not mind a more concerted effort to migrate to whatever style we pick, 
which was notably lacking last time around. Then the jarring inconsistencies 
would go away, and we could all get back to complaining about content and not 
style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57896#1406336 , @zturner wrote:

>


...

I can't argue with anything you say.. but I guess to reinforce your point 
introducing what is effectively a 3rd style would likely cause even more 
jarring...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1405334 , @MyDeveloperDay 
wrote:

> In D57896#1402280 , @zturner wrote:
>
> > Since someone already accepted this, I suppose I should mark require 
> > changes to formalize my dissent
>
>
> As it was Chris @lattner who accepted it, is your request for changes just 
> based on the fact that it doesn't fit LLDB style?


(Side note, but I think everyones' opinions hold the same weight with regards 
to issues like this, and that is in part why changes like this are so difficult 
to move forward with. Because it takes a lot of consensus, not just one person, 
to drive a change.)

To answer your question: In a way, yes.  To be clear, I don't actually care 
what the style we end up with is and I think arguing over which specific style 
we end up adopting is a silly argument.  No style is going to be aesthetically 
pleasing to everyone, and I conjecture that any style we choose will have just 
as many people who dislike it as there are that like it.  A coding style should 
serve exactly two purposes (in this order of importance): 1) Consistency across 
codebase, and 2) Visually distinguish semantically names that refer to 
semantically different things.

As long as it satisfies those two things, the specific choice of style is 
almost incosequential.

My objection is based on the fact adopting LLDB's style makes #1 
**significantly better** at literally no incremental cost, while maintaining 
#2.  So, the benefit of changing to literally any other style would be dwarfed 
by the benefit of changing to this particular style, because we would get 
instant consistency across a large swath of code.

If someone wants to propose a mass change of LLDB's names, I would actually be 
fine with that, but I suspect that will be just as difficult to drive, and so 
the path of least resistance here is to just use it and move on with our lives.

> I was trying to find where the LLDB coding style was documented but could 
> only find this 
> https://llvm.org/svn/llvm-project/lldb/branches/release_36/www/lldb-coding-conventions.html,
>  seemingly this file has been move/removed around release 3.9.
> 
> But reading that link its seems unlikely to find a concencous between either 
> naming conventions or formatting style between LLDB and the rest of LLVM, 
> unless of course the solution would be to adopt LLDB style completely (for 
> which I'd suspect there would be counter objections)

If there are counter objections, I'd like to hear them.  "I'm not a fan of that 
style" is not really a strong counter-objection in my opinion, because if we 
require a unanimous consensus on the most aesthetically pleasing style, I'm 
pretty sure nothing will ever happen.  After all, I'm not a huge fan of LLDB's 
style myself.  But as with any coding standard, you just deal with it.

> If that isn't a reality is blocking the rest of the LLVM community from 
> relieving some of their eye strain an acceptable solution?

Inconsistency is worse than eye strain, because it *causes* eye strain, as well 
as discourages people from contributing to the code at all.  Anyone who has 
worked on both LLDB and LLVM can attest to how jarring the shift is moving back 
and forth between them, and that is a much more serious problem than a subset 
of developers who don't like something and another subset who do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57896#1402280 , @zturner wrote:

> Since someone already accepted this, I suppose I should mark require changes 
> to formalize my dissent


As it was Chris @lattner who accepted it, is your request for changes just 
based on the fact that it doesn't fit LLDB style?

I was trying to find where the LLDB coding style was documented but could only 
find this 
https://llvm.org/svn/llvm-project/lldb/branches/release_36/www/lldb-coding-conventions.html,
 seemingly this file has been move/removed around release 3.9.

But reading that link its seems unlikely to find a concencous between either 
naming conventions or formatting style between LLDB and the rest of LLVM, 
unless of course the solution would be to adopt LLDB style completely (for 
which I'd suspect there would be counter objections)

If that isn't a reality is blocking the rest of the LLVM community from 
relieving some of their eye strain an acceptable solution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.

Since someone already accepted this, I suppose I should mark require changes to 
formalize my dissent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1402194 , @lattner wrote:

> > Changed recommendation for acronyms from lower case to upper case, as 
> > suggested by several responses to the RFC.
>
> I haven't been following the discussion closely - why is this the preferred 
> direction?  I don't think that things like "Basicblock *bb" or "MachineInstr 
> *mi" will be confusing, and going towards a consistently leading lower case 
> letter seems simple and preferable.
>
> -Chris


I don’t think we should use this review as evidence of consensus.  For example, 
I’m going to be against any change that doesn’t bring us closer to LLDB’s style 
of `lower_case` simply on the grounds that a move which brings us farther away 
from global consistency is strictly worse than one which brings us closer, 
despite ones personal aesthetic preferences.

And so far, I don’t really see that addressed here (or in the thread)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

In D57896#1402194 , @lattner wrote:

> > Changed recommendation for acronyms from lower case to upper case, as 
> > suggested by several responses to the RFC.
>
> I haven't been following the discussion closely - why is this the preferred 
> direction?  I don't think that things like "Basicblock *bb" or "MachineInstr 
> *mi" will be confusing, and going towards a consistently leading lower case 
> letter seems simple and preferable.


Maybe I misunderstood you 
(http://lists.llvm.org/pipermail/llvm-dev/2019-February/130223.html):

>> Maybe there should be an exception that variable names that start with an 
>> acronym still should start with an upper case letter?
>  That would also be fine with me - it could push such a debate down the road, 
> and is definitely important for a transition period anyway.

Also Chandler 
(http://lists.llvm.org/pipermail/llvm-dev/2019-February/130313.html):

> FWIW, I suspect separating the transition of our acronyms from the transition 
> of identifiers with non-acronym words may be an effective way to chip away at 
> the transition cost... Definitely an area that people who really care about 
> this should look at carefully.

And Sanjoy (kind of) 
(http://lists.llvm.org/pipermail/llvm-dev/2019-February/130304.html):

> maybe a gradual transition plan could be to allow these upper case acronyms 
> for specific classes?

I agree that lower case acronyms would ultimately be more consistent, but given 
where we are it seems more achievable to only change the rule for non-acronyms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

> Changed recommendation for acronyms from lower case to upper case, as 
> suggested by several responses to the RFC.

I haven't been following the discussion closely - why is this the preferred 
direction?  I don't think that things like "Basicblock *bb" or "MachineInstr 
*mi" will be confusing, and going towards a consistently leading lower case 
letter seems simple and preferable.

-Chris


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 187344.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

Files:
  .clang-tidy
  clang/.clang-tidy
  llvm/.clang-tidy
  llvm/docs/CodingStandards.rst

Index: llvm/docs/CodingStandards.rst
===
--- llvm/docs/CodingStandards.rst
+++ llvm/docs/CodingStandards.rst
@@ -311,13 +311,13 @@
 
.. code-block:: c++
 
- Object.emitName(nullptr);
+ object.emitName(nullptr);
 
An in-line C-style comment makes the intent obvious:
 
.. code-block:: c++
 
- Object.emitName(/*Prefix=*/nullptr);
+ object.emitName(/*prefix=*/nullptr);
 
 Commenting out large blocks of code is discouraged, but if you really have to do
 this (for documentation purposes or as a suggestion for debug printing), use
@@ -355,8 +355,8 @@
 
 .. code-block:: c++
 
-  /// Sets the xyzzy property to \p Baz.
-  void setXyzzy(bool Baz);
+  /// Sets the xyzzy property to \p baz.
+  void setXyzzy(bool baz);
 
 A documentation comment that uses all Doxygen features in a preferred way:
 
@@ -364,18 +364,18 @@
 
   /// Does foo and bar.
   ///
-  /// Does not do foo the usual way if \p Baz is true.
+  /// Does not do foo the usual way if \p baz is true.
   ///
   /// Typical usage:
   /// \code
-  ///   fooBar(false, "quux", Res);
+  ///   fooBar(false, "quux", res);
   /// \endcode
   ///
-  /// \param Quux kind of foo to do.
+  /// \param quux kind of foo to do.
   /// \param [out] Result filled with bar sequence on foo success.
   ///
   /// \returns true on success.
-  bool fooBar(bool Baz, StringRef Quux, std::vector );
+  bool fooBar(bool baz, StringRef quux, std::vector );
 
 Don't duplicate the documentation comment in the header file and in the
 implementation file.  Put the documentation comments for public APIs into the
@@ -603,7 +603,7 @@
 
   foo({a, b, c}, {1, 2, 3});
 
-  llvm::Constant *Mask[] = {
+  llvm::Constant *mask[] = {
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 0),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 1),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 2)};
@@ -633,7 +633,7 @@
 
 .. code-block:: c++
 
-  if (V = getValue()) {
+  if (v = getValue()) {
 ...
   }
 
@@ -644,7 +644,7 @@
 
 .. code-block:: c++
 
-  if ((V = getValue())) {
+  if ((v = getValue())) {
 ...
   }
 
@@ -736,7 +736,7 @@
   class Foo;
 
   // Breaks mangling in MSVC.
-  struct Foo { int Data; };
+  struct Foo { int data; };
 
 * As a rule of thumb, ``struct`` should be kept to structures where *all*
   members are declared public.
@@ -746,17 +746,17 @@
   // Foo feels like a class... this is strange.
   struct Foo {
   private:
-int Data;
+int data;
   public:
-Foo() : Data(0) { }
-int getData() const { return Data; }
-void setData(int D) { Data = D; }
+Foo() : data(0) { }
+int getData() const { return data; }
+void setData(int d) { data = d; }
   };
 
   // Bar isn't POD, but it does look like a struct.
   struct Bar {
-int Data;
-Bar() : Data(0) { }
+int data;
+Bar() : data(0) { }
   };
 
 Do not use Braced Initializer Lists to Call a Constructor
@@ -780,7 +780,7 @@
 Foo(std::string filename);
 
 // Construct a Foo by looking up the Nth element of some global data ...
-Foo(int N);
+Foo(int n);
 
 // ...
   };
@@ -789,7 +789,7 @@
   std::fill(foo.begin(), foo.end(), Foo("name"));
 
   // The pair is just being constructed like an aggregate, use braces.
-  bar_map.insert({my_key, my_value});
+  bar_map.insert({myKey, myValue});
 
 If you use a braced initializer list when initializing a variable, use an equals before the open curly brace:
 
@@ -821,15 +821,15 @@
 .. code-block:: c++
 
   // Typically there's no reason to copy.
-  for (const auto  : Container) { observe(Val); }
-  for (auto  : Container) { Val.change(); }
+  for (const auto  : container) { observe(val); }
+  for (auto  : container) { val.change(); }
 
   // Remove the reference if you really want a new copy.
-  for (auto Val : Container) { Val.change(); saveSomewhere(Val); }
+  for (auto val : container) { val.change(); saveSomewhere(val); }
 
   // Copy pointers, but make it clear that they're pointers.
-  for (const auto *Ptr : Container) { observe(*Ptr); }
-  for (auto *Ptr : Container) { Ptr->change(); }
+  for (const auto *ptr : container) { observe(*ptr); }
+  for (auto *ptr : container) { ptr->change(); }
 
 Beware of non-determinism due to ordering of pointers
 ^
@@ -968,9 +968,9 @@
 
 .. code-block:: c++
 
-  Value *doSomething(Instruction *I) {
-if (!I->isTerminator() &&
-I->hasOneUse() && doOtherThing(I)) {
+  Value *doSomething(Instruction *instruction) {
+if (!instruction->isTerminator() &&
+instruction->hasOneUse() && 

[PATCH] D57896: Variable names rule

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki added inline comments.



Comment at: llvm/docs/CodingStandards.rst:1065
   case 'J': {
-if (Signed) {
-  Type = Context.getsigjmp_bufType();
-  if (Type.isNull()) {
-Error = ASTContext::GE_Missing_sigjmp_buf;
+if (signed) {
+  type = context.getsigjmp_bufType();

signed is a keyword, it can't be used as a variable name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 187339.
michaelplatings added a comment.

Changed recommendation for acronyms from lower case to upper case, as suggested 
by several responses to the RFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

Files:
  .clang-tidy
  clang/.clang-tidy
  llvm/.clang-tidy
  llvm/docs/CodingStandards.rst

Index: llvm/docs/CodingStandards.rst
===
--- llvm/docs/CodingStandards.rst
+++ llvm/docs/CodingStandards.rst
@@ -311,13 +311,13 @@
 
.. code-block:: c++
 
- Object.emitName(nullptr);
+ object.emitName(nullptr);
 
An in-line C-style comment makes the intent obvious:
 
.. code-block:: c++
 
- Object.emitName(/*Prefix=*/nullptr);
+ object.emitName(/*prefix=*/nullptr);
 
 Commenting out large blocks of code is discouraged, but if you really have to do
 this (for documentation purposes or as a suggestion for debug printing), use
@@ -355,8 +355,8 @@
 
 .. code-block:: c++
 
-  /// Sets the xyzzy property to \p Baz.
-  void setXyzzy(bool Baz);
+  /// Sets the xyzzy property to \p baz.
+  void setXyzzy(bool baz);
 
 A documentation comment that uses all Doxygen features in a preferred way:
 
@@ -364,18 +364,18 @@
 
   /// Does foo and bar.
   ///
-  /// Does not do foo the usual way if \p Baz is true.
+  /// Does not do foo the usual way if \p baz is true.
   ///
   /// Typical usage:
   /// \code
-  ///   fooBar(false, "quux", Res);
+  ///   fooBar(false, "quux", res);
   /// \endcode
   ///
-  /// \param Quux kind of foo to do.
+  /// \param quux kind of foo to do.
   /// \param [out] Result filled with bar sequence on foo success.
   ///
   /// \returns true on success.
-  bool fooBar(bool Baz, StringRef Quux, std::vector );
+  bool fooBar(bool baz, StringRef quux, std::vector );
 
 Don't duplicate the documentation comment in the header file and in the
 implementation file.  Put the documentation comments for public APIs into the
@@ -603,7 +603,7 @@
 
   foo({a, b, c}, {1, 2, 3});
 
-  llvm::Constant *Mask[] = {
+  llvm::Constant *mask[] = {
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 0),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 1),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 2)};
@@ -633,7 +633,7 @@
 
 .. code-block:: c++
 
-  if (V = getValue()) {
+  if (v = getValue()) {
 ...
   }
 
@@ -644,7 +644,7 @@
 
 .. code-block:: c++
 
-  if ((V = getValue())) {
+  if ((v = getValue())) {
 ...
   }
 
@@ -736,7 +736,7 @@
   class Foo;
 
   // Breaks mangling in MSVC.
-  struct Foo { int Data; };
+  struct Foo { int data; };
 
 * As a rule of thumb, ``struct`` should be kept to structures where *all*
   members are declared public.
@@ -746,17 +746,17 @@
   // Foo feels like a class... this is strange.
   struct Foo {
   private:
-int Data;
+int data;
   public:
-Foo() : Data(0) { }
-int getData() const { return Data; }
-void setData(int D) { Data = D; }
+Foo() : data(0) { }
+int getData() const { return data; }
+void setData(int d) { data = d; }
   };
 
   // Bar isn't POD, but it does look like a struct.
   struct Bar {
-int Data;
-Bar() : Data(0) { }
+int data;
+Bar() : data(0) { }
   };
 
 Do not use Braced Initializer Lists to Call a Constructor
@@ -780,7 +780,7 @@
 Foo(std::string filename);
 
 // Construct a Foo by looking up the Nth element of some global data ...
-Foo(int N);
+Foo(int n);
 
 // ...
   };
@@ -789,7 +789,7 @@
   std::fill(foo.begin(), foo.end(), Foo("name"));
 
   // The pair is just being constructed like an aggregate, use braces.
-  bar_map.insert({my_key, my_value});
+  bar_map.insert({myKey, myValue});
 
 If you use a braced initializer list when initializing a variable, use an equals before the open curly brace:
 
@@ -821,15 +821,15 @@
 .. code-block:: c++
 
   // Typically there's no reason to copy.
-  for (const auto  : Container) { observe(Val); }
-  for (auto  : Container) { Val.change(); }
+  for (const auto  : container) { observe(val); }
+  for (auto  : container) { val.change(); }
 
   // Remove the reference if you really want a new copy.
-  for (auto Val : Container) { Val.change(); saveSomewhere(Val); }
+  for (auto val : container) { val.change(); saveSomewhere(val); }
 
   // Copy pointers, but make it clear that they're pointers.
-  for (const auto *Ptr : Container) { observe(*Ptr); }
-  for (auto *Ptr : Container) { Ptr->change(); }
+  for (const auto *ptr : container) { observe(*ptr); }
+  for (auto *ptr : container) { ptr->change(); }
 
 Beware of non-determinism due to ordering of pointers
 ^
@@ -968,9 +968,9 @@
 
 .. code-block:: c++
 
-  Value *doSomething(Instruction *I) {
-if (!I->isTerminator() &&
-I->hasOneUse() && 

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

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



Comment at: llvm/docs/CodingStandards.rst:1195
+  be camel case, and start with a lower case letter (e.g. ``leader`` or
+  ``boats``). It is also acceptable to use ``UpperCamelCase`` for consistency
+  with existing code.

rupprecht wrote:
> It would be nice for this section to be expanded a bit, just to avoid 
> inevitable code review churn, e.g. if I'm adding 50 lines to a 200 line file, 
> am I allowed to change the existing var names elsewhere in the file or 
> method, or is that outside the scope of my change? If I'm reviewing that 
> patch, do I tell the author they have to be consistent and revert other 
> changes? etc.
> 
> Is there any plan to use clang-tidy to do a global cleanup, or is this going 
> to be a totally ad-hoc migration -- variables use the new scheme only when 
> the code is updated?
I've had a go at expanding it. Please let me know if you have other suggestions.

> Is there any plan to use clang-tidy to do a global cleanup, or is this going 
> to be a totally ad-hoc migration -- variables use the new scheme only when 
> the code is updated?

The latter. Given that the code doesn't keep to the existing .clang-tidy rules 
I'm not optimistic that we could persuade code owners to start now. That's not 
to say it couldn't happen eventually, but my aim at this point in time is to 
make it easier to use good variable names and I don't want perfect to be the 
enemy of better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 187252.
michaelplatings added a comment.

Update .clang-tidy files to use aNy_CasE until camelBackOrCase is available.
Add more guidance around acronyms.
Add more guidance around consistency with existing CamelCase variable names.
Change other code examples to camelBack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896

Files:
  .clang-tidy
  clang/.clang-tidy
  llvm/.clang-tidy
  llvm/docs/CodingStandards.rst

Index: llvm/docs/CodingStandards.rst
===
--- llvm/docs/CodingStandards.rst
+++ llvm/docs/CodingStandards.rst
@@ -311,13 +311,13 @@
 
.. code-block:: c++
 
- Object.emitName(nullptr);
+ object.emitName(nullptr);
 
An in-line C-style comment makes the intent obvious:
 
.. code-block:: c++
 
- Object.emitName(/*Prefix=*/nullptr);
+ object.emitName(/*prefix=*/nullptr);
 
 Commenting out large blocks of code is discouraged, but if you really have to do
 this (for documentation purposes or as a suggestion for debug printing), use
@@ -355,8 +355,8 @@
 
 .. code-block:: c++
 
-  /// Sets the xyzzy property to \p Baz.
-  void setXyzzy(bool Baz);
+  /// Sets the xyzzy property to \p baz.
+  void setXyzzy(bool baz);
 
 A documentation comment that uses all Doxygen features in a preferred way:
 
@@ -364,18 +364,18 @@
 
   /// Does foo and bar.
   ///
-  /// Does not do foo the usual way if \p Baz is true.
+  /// Does not do foo the usual way if \p baz is true.
   ///
   /// Typical usage:
   /// \code
-  ///   fooBar(false, "quux", Res);
+  ///   fooBar(false, "quux", res);
   /// \endcode
   ///
-  /// \param Quux kind of foo to do.
+  /// \param quux kind of foo to do.
   /// \param [out] Result filled with bar sequence on foo success.
   ///
   /// \returns true on success.
-  bool fooBar(bool Baz, StringRef Quux, std::vector );
+  bool fooBar(bool baz, StringRef quux, std::vector );
 
 Don't duplicate the documentation comment in the header file and in the
 implementation file.  Put the documentation comments for public APIs into the
@@ -568,16 +568,16 @@
 .. code-block:: c++
 
   dyn_switch(V->stripPointerCasts(),
- [] (PHINode *PN) {
+ [] (PHINode *phiNode) {
// process phis...
  },
- [] (SelectInst *SI) {
+ [] (SelectInst *selectInst) {
// process selects...
  },
- [] (LoadInst *LI) {
+ [] (LoadInst *loadInst) {
// process loads...
  },
- [] (AllocaInst *AI) {
+ [] (AllocaInst *allocaInst) {
// process allocas...
  });
 
@@ -603,7 +603,7 @@
 
   foo({a, b, c}, {1, 2, 3});
 
-  llvm::Constant *Mask[] = {
+  llvm::Constant *mask[] = {
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 0),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 1),
   llvm::ConstantInt::get(llvm::Type::getInt32Ty(getLLVMContext()), 2)};
@@ -633,7 +633,7 @@
 
 .. code-block:: c++
 
-  if (V = getValue()) {
+  if (v = getValue()) {
 ...
   }
 
@@ -644,7 +644,7 @@
 
 .. code-block:: c++
 
-  if ((V = getValue())) {
+  if ((v = getValue())) {
 ...
   }
 
@@ -736,7 +736,7 @@
   class Foo;
 
   // Breaks mangling in MSVC.
-  struct Foo { int Data; };
+  struct Foo { int data; };
 
 * As a rule of thumb, ``struct`` should be kept to structures where *all*
   members are declared public.
@@ -746,17 +746,17 @@
   // Foo feels like a class... this is strange.
   struct Foo {
   private:
-int Data;
+int data;
   public:
-Foo() : Data(0) { }
-int getData() const { return Data; }
-void setData(int D) { Data = D; }
+Foo() : data(0) { }
+int getData() const { return data; }
+void setData(int d) { data = d; }
   };
 
   // Bar isn't POD, but it does look like a struct.
   struct Bar {
-int Data;
-Bar() : Data(0) { }
+int data;
+Bar() : data(0) { }
   };
 
 Do not use Braced Initializer Lists to Call a Constructor
@@ -780,7 +780,7 @@
 Foo(std::string filename);
 
 // Construct a Foo by looking up the Nth element of some global data ...
-Foo(int N);
+Foo(int n);
 
 // ...
   };
@@ -789,7 +789,7 @@
   std::fill(foo.begin(), foo.end(), Foo("name"));
 
   // The pair is just being constructed like an aggregate, use braces.
-  bar_map.insert({my_key, my_value});
+  bar_map.insert({myKey, myValue});
 
 If you use a braced initializer list when initializing a variable, use an equals before the open curly brace:
 
@@ -821,15 +821,15 @@
 .. code-block:: c++
 
   // Typically there's no reason to copy.
-  for (const auto  : Container) { observe(Val); }
-  for (auto  : Container) { Val.change(); }
+  for (const auto  : container) { observe(val); }
+  for (auto  : container) { val.change(); }
 
   // Remove 

[PATCH] D57896: Variable names rule

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D57896#1391925 , 
@hubert.reinterpretcast wrote:

> In D57896#1391611 , @zturner wrote:
>
> > Is this actually any better?  Whereas before we can’t differentiate type 
> > names and variable names, under this proposal we can’t differentiate type 
> > names and function names.  So it seems a bit of “6 of 1, half dozen of 
> > another”
>
>
> Perhaps you mistyped? The proposal does not change the status quo of either 
> type names nor function names. If you mean that we can't differentiate 
> variable names and function names, then it seems worthwhile to point out that 
> the actual letters (not just the case of said letters) matter too. Whereas 
> the guidelines state that types and variables should have names that are 
> nouns, the guidelines state that functions should have names that are verb 
> phrases.


There is still overlap, e.g. "process" can be a noun (a Linux process) or a 
verb (to process something)

I think it should also be pointed out there is not zero overhead -- it's not a 
lot (at least for native English speakers, which many LLVM developers are not), 
but determining if a word is a verb or a noun is harder than looking at the 
casing. Small, but worth observing.

A different convention, e.g. `lower_case`, avoids this. Personally, I'd prefer 
that, but I'm also fine with lowerCamelCase just so we can stop using 
UpperCamelCase.




Comment at: llvm/docs/CodingStandards.rst:1195
+  be camel case, and start with a lower case letter (e.g. ``leader`` or
+  ``boats``). It is also acceptable to use ``UpperCamelCase`` for consistency
+  with existing code.

It would be nice for this section to be expanded a bit, just to avoid 
inevitable code review churn, e.g. if I'm adding 50 lines to a 200 line file, 
am I allowed to change the existing var names elsewhere in the file or method, 
or is that outside the scope of my change? If I'm reviewing that patch, do I 
tell the author they have to be consistent and revert other changes? etc.

Is there any plan to use clang-tidy to do a global cleanup, or is this going to 
be a totally ad-hoc migration -- variables use the new scheme only when the 
code is updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> Sounds good to me. I see that you've made D57966 
>  a child of this issue, but we could swap 
> the dependency around so that once your patch is applied I can update this 
> patch to use `camelBackOrCase`.

I'm OK if we want to do that, but its very much a circular dependency, I don't 
want to land D57966: [clang-tidy] add camelBackOrCase casing style to 
readability-identifier-naming to support change to variable naming policy (if 
adopted) , unless this whole variableName 
suggestion is accepted (which I think is a good idea).. I think your suggestion 
warrants being the driver, lets do the clang-tidy change and subsequent 
.clang-tidy changes on another revision post acceptance of both.

FWIW, I agree with the comments that the function name should be differentiated 
from the variable by the use of a verbs, I've spent too much time in my career 
grepping for the work `type` in code

  Type type = type();

I think

  Type type = getType();

or

  Type objectType = getType();

adds some increased levels of clarity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

In D57896#1390517 , @MyDeveloperDay 
wrote:

> Should we come up with a new style?  say `UpperOrLowerCamelCase`,   I don't 
> mind going and doing that in the readability-identifier-naming check, given 
> that I just wrote up all the Options for that check 
> https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html
>  in D56563: [clang-tidy]  add options documentation to 
> readability-identifier-naming checker 


Sounds good to me. I see that you've made D57966 
 a child of this issue, but we could swap the 
dependency around so that once your patch is applied I can update this patch to 
use `camelBackOrCase`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D57896#1391611 , @zturner wrote:

> Is this actually any better?  Whereas before we can’t differentiate type 
> names and variable names, under this proposal we can’t differentiate type 
> names and function names.  So it seems a bit of “6 of 1, half dozen of 
> another”


Perhaps you mistyped? The proposal does not change the status quo of either 
type names nor function names. If you mean that we can't differentiate variable 
names and function names, then it seems worthwhile to point out that the actual 
letters (not just the case of said letters) matter too. Whereas the guidelines 
state that types and variables should have names that are nouns, the guidelines 
state that functions should have names that are verb phrases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Is this actually any better?  Whereas before we can’t differentiate type names 
and variable names, under this proposal we can’t differentiate type names and 
function names.  So it seems a bit of “6 of 1, half dozen of another”


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> I personally would be happy to change the settings from `camelBack` to 
> `aNy_CasE`.

Should we come up with a new style?  UpperOrLowerCamelCase,   I don't mind 
going and doing that in the readability-identifier-naming check, given that I 
just wrote up all the Options for that check 
https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html
 in D56563: [clang-tidy]  add options documentation to 
readability-identifier-naming checker 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

In D57896#1389067 , @lebedev.ri wrote:

> 1. Does clang-tidy warn on every single existing variable now?
> 2. It might be best to give this more visibility, by submitting a mail to 
> llvm-dev, with a **noticeable** subject, like "RFC: changing variable naming 
> rules in LLVM codebase"




1. Pretty much. Previously clang-tidy gave 56,787 unique warnings. After this 
patch it gives 361,382 unique warnings. I personally would be happy to change 
the settings from `camelBack` to `aNy_CasE`.

2. Done: http://lists.llvm.org/pipermail/llvm-dev/2019-February/130083.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

I am very much +1 on this.  That said, this isn't the sort of thing we just use 
patch review for.  Please agitate a robust discussion about this on llvm-dev.  
:-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

I am generally in favour of this direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

does the readability-identifier-naming check need to be changed to support 
multiple allowed case types?

  - key: readability-identifier-naming.VariableCase
  value:camelBack,CamelBack


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.
Herald added a project: LLVM.

In D57896#1389067 , @lebedev.ri wrote:

> 2. It might be best to give this more visibility, by submitting a mail to 
> llvm-dev, with a **noticeable** subject, like "RFC: changing variable naming 
> rules in LLVM codebase"


+1. I know the discussion took place there originally, but "RFC" will catch 
more people's eyes. Also the prior discussion had some digressions (ahem) which 
a new RFC can be more focused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57896#1389046 , @michaelplatings 
wrote:

> In D57896#1389042 , @lebedev.ri 
> wrote:
>
> > Pretty sure this patch should have gone to llvm-commits, not cfe-commits.
>
>
> I just set the repository, Phabricator did the rest - apparently the magic 
> isn't working so well.




1. Does clang-tidy warn on every single existing variable now?
2. It might be best to give this more visibility, by submitting a mail to 
llvm-dev, with a **noticeable** subject, like "RFC: changing variable naming 
rules in LLVM codebase"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

In D57896#1389042 , @lebedev.ri wrote:

> Pretty sure this patch should have gone to llvm-commits, not cfe-commits.


I just set the repository, Phabricator did the rest - apparently the magic 
isn't working so well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Pretty sure this patch should have gone to llvm-commits, not cfe-commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57896: Variable names rule

[Michael Platings via Phabricator via cfe-commits]

michaelplatings created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Following discussion 
 and 
general agreement that the current naming rule for variables is not ideal, this 
patch switches the naming rule to make `lowerCamelCase` the standard, 
consistent with a prior RFC 
.

Given that over 450,000 variables are currently named in `UpperCamelCase`, the 
rule also permits using that form for consistency with existing code. I can't 
see a way to express that in .clang-tidy files.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57896

Files:
  .clang-tidy
  clang/.clang-tidy
  llvm/.clang-tidy
  llvm/docs/CodingStandards.rst

Index: llvm/docs/CodingStandards.rst
===
--- llvm/docs/CodingStandards.rst
+++ llvm/docs/CodingStandards.rst
@@ -1191,8 +1191,9 @@
   nouns and start with an upper-case letter (e.g. ``TextFileReader``).
 
 * **Variable names** should be nouns (as they represent state).  The name should
-  be camel case, and start with an upper case letter (e.g. ``Leader`` or
-  ``Boats``).
+  be camel case, and start with a lower case letter (e.g. ``leader`` or
+  ``boats``). It is also acceptable to use ``UpperCamelCase`` for consistency
+  with existing code.
   
 * **Function names** should be verb phrases (as they represent actions), and
   command-like function should be imperative.  The name should be camel case,
@@ -1232,16 +1233,22 @@
 
   class VehicleMaker {
 ...
-Factory F;// Bad -- abbreviation and non-descriptive.
-Factory Factory;  // Better.
-Factory TireFactory;  // Even better -- if VehicleMaker has more than one
-// kind of factories.
+Factory f;// Bad -- abbreviation and non-descriptive.
+Factory factory;  // Better.
+Factory tireFactory;  // Even better -- if VehicleMaker has more than
+// one kind of factory.
   };
 
-  Vehicle makeVehicle(VehicleType Type) {
-VehicleMaker M; // Might be OK if having a short life-span.
-Tire Tmp1 = M.makeTire();   // Bad -- 'Tmp1' provides no information.
-Light Headlight = M.makeLight("head");  // Good -- descriptive.
+  Vehicle makeVehicle(VehicleType type) {
+// Reusing the type name in lowerCamelCase form is often a good way to get
+// a suitable variable name.
+VehicleMaker vehicleMaker;
+
+// Bad -- 'tmp1' provides no information.
+Tire tmp1 = vehicleMaker.makeTire();
+
+// Good -- descriptive.
+Light headlight = vehicleMaker.makeLight("head");
 ...
   }
 
Index: llvm/.clang-tidy
===
--- llvm/.clang-tidy
+++ llvm/.clang-tidy
@@ -7,11 +7,11 @@
   - key: readability-identifier-naming.FunctionCase
 value:   camelBack
   - key: readability-identifier-naming.MemberCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.ParameterCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.UnionCase
 value:   CamelCase
   - key: readability-identifier-naming.VariableCase
-value:   CamelCase
+value:   camelBack
 
Index: clang/.clang-tidy
===
--- clang/.clang-tidy
+++ clang/.clang-tidy
@@ -12,11 +12,11 @@
   - key: readability-identifier-naming.FunctionCase
 value:   camelBack
   - key: readability-identifier-naming.MemberCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.ParameterCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.UnionCase
 value:   CamelCase
   - key: readability-identifier-naming.VariableCase
-value:   CamelCase
+value:   camelBack
 
Index: .clang-tidy
===
--- .clang-tidy
+++ .clang-tidy
@@ -7,11 +7,11 @@
   - key: readability-identifier-naming.FunctionCase
 value:   camelBack
   - key: readability-identifier-naming.MemberCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.ParameterCase
-value:   CamelCase
+value:   camelBack
   - key: readability-identifier-naming.UnionCase
 value:   CamelCase
   - key: readability-identifier-naming.VariableCase
-value:   CamelCase
+value: