[PATCH] D126247: [clang-tidy][doc] Document readability-indentifier-naming resolution order and examples

Tue Jun 21 15:06:03 PDT 2022

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