KazNX marked 17 inline comments as done.
KazNX added inline comments.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2750-2752
+where an individual identifier can fall into several classifications. Below
+is a list of the classifications supported by ``readability-identifier-naming``
+presented in the order in which the classifications are resolved. Some
----------------
whisperity wrote:
> So is this resolution order the same for //every Decl//?
Not as I found it. `IdentifierNamingCheck::findStyleKind()` is laid out mostly
in an ordered fashion, but some checks necessarily repeat either in different
context or because of shared semantics. The best example I have is the `struct`
vs `class` relationship. A `struct` will first be idenified as `SK_Struct`, but
if there's no naming for that, then it will try `SK_Class`. Meanwhile, a
`class` does the same thing in the opposite order; first `SK_Class` then
`SK_Struct`.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2742
double dValueDouble = 0.0;
ULONG ulValueUlong = 0;
----------------
LegalizeAdulthood wrote:
> Phabricator says there is no context available. Did you generate this diff
> with `git diff -U999999 main`?
No sorry. Will fix on next submission.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2756
+matched. This occurs when the semantics of the identifier match and there is a
+valid option for the classification present in the ``.clang-tidy`` file - e.g.,
+``readability-identifier-naming.AbstractClassCase``.
----------------
LegalizeAdulthood wrote:
> Does it have to be in the `.clang-tidy` file, or is it just a matter of
> setting options?
I've changed the wording.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2772
+ - Class ``[class, struct]``
+ - Struct ``[class]``
+ - Union ``[union]``
----------------
LegalizeAdulthood wrote:
> Why is `Struct` listed twice?
This is a reflection of what actually happes. For a `struct`, it tries to
resovle first as a `struct`, then as a `class`. Meanwhile, for a `class` it
tries `class` first then `struct`. Note the accompanying keywords.
This is also addressed in the paragraph above the list.
> Some classifications appear multiple times as they can be checked in different
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2786
+ - <parameters>
+ - ConstexprVariable ``[constexpr]``
+ - ``[const]``
----------------
LegalizeAdulthood wrote:
> I would have expected `ConstExprVariable` instead?
That's not the case I'm seeing in code or in the documentation. See
https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-constexprvariablecase
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2857
+attempt disambiguation within the context of this document. For example,
+``<member-variables>`` identifiers that clang tidy is currently looking only at
+member variables.
----------------
LegalizeAdulthood wrote:
> Eugene.Zelenko wrote:
> > Ditto.
> `identifies`, not `identifiers`
Intentionaly. I'm using the noun, not the verb.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2880
+
+The code snippet below[1]_ serves as an exhaustive example of various
+identifiers the ``readability-identifier-naming`` check is able to classify.
----------------
LegalizeAdulthood wrote:
> Should we be linking to ephemeral gists that can disappear?
I'm changing this link to a full repo.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2880
+
+The code snippet below[1]_ serves as an exhaustive example of various
+identifiers the ``readability-identifier-naming`` check is able to classify.
----------------
whisperity wrote:
> KazNX wrote:
> > LegalizeAdulthood wrote:
> > > Should we be linking to ephemeral gists that can disappear?
> > I'm changing this link to a full repo.
> Are the contents of the file linked and the one in the documentation here the
> exact same? In that case I think if the author gives rights for LLVM to use
> their work, we can get rid of the link, as it serves no additional value.
Yeah, I'm no expert on all this, but I am the author of both. If I had
submitted a patch first then I've wouldn't have included the link.
To answer directly, the contents has only minor differences.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3054
+ // @param param Parameter
+ int func(std::string *const str_ptr, const std::string &str, int
*ptr_param, int param)
+ {
----------------
LegalizeAdulthood wrote:
> This covers "const pointer", but what about "pointer to const"?
> e.g. `std::string const *str_ptr` how is this handled?
I can add that. Empirically that turned out a `ConstantParameter`.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3054
+ // @param param Parameter
+ int func(std::string *const str_ptr, const std::string &str, int
*ptr_param, int param)
+ {
----------------
KazNX wrote:
> LegalizeAdulthood wrote:
> > This covers "const pointer", but what about "pointer to const"?
> > e.g. `std::string const *str_ptr` how is this handled?
> I can add that. Empirically that turned out a `ConstantParameter`.
I was wrong. There's no `const` association.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3120
+ // GlobalConstantPointer, GlobalConstant, Constant, GlobalPointer,
GlobalVariable, Variable
+ int *const global_const_ptr = nullptr;
+
----------------
LegalizeAdulthood wrote:
> Same, pointer to `const`?
I'm adding a few more variants including `static`.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3127-3129
+ // StaticConstant does not actually trip for this declaration despite the
documentation indicating that it should.
+ // StaticConstant does not appear to trip for anything. Reading the code,
it seems that StaticConstant logic is in the
+ // wrong place and the conditions cannot be met.
----------------
LegalizeAdulthood wrote:
> Is this really a bug report disguised as a doc comment?
Yes it is. Sorry, was captured from my original document. I agree it doesn't
belong here.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:3127-3129
+ // StaticConstant does not actually trip for this declaration despite the
documentation indicating that it should.
+ // StaticConstant does not appear to trip for anything. Reading the code,
it seems that StaticConstant logic is in the
+ // wrong place and the conditions cannot be met.
----------------
KazNX wrote:
> LegalizeAdulthood wrote:
> > Is this really a bug report disguised as a doc comment?
> Yes it is. Sorry, was captured from my original document. I agree it doesn't
> belong here.
I've added a new bug report for this:
https://github.com/llvm/llvm-project/issues/55705
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126247/new/
https://reviews.llvm.org/D126247
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
