[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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/

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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 =

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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:

[PATCH] D57896: Variable names rule

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 =

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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**

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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 >

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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.

[PATCH] D57896: Variable names rule

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 ___

[PATCH] D57896: Variable names rule

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