Page MenuHomePhabricator

[clang-tidy][doc] Document readability-indentifier-naming resolution order and examples
Needs RevisionPublic

Authored by KazNX on May 23 2022, 3:28 PM.

Details

Summary

Added extensive documentation to identify the order in which readability-identifier-naming attempts to resolve its classifications. This is followed by an exhaustive code example covering all identifiers commented with their appropriate classifications. This seeks to provide improved information for those seeking to define a particular naming standard.

Diff Detail

Event Timeline

KazNX created this revision.May 23 2022, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 3:28 PM
Herald added a subscriber: rnkovacs. · View Herald Transcript
KazNX requested review of this revision.May 23 2022, 3:28 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
2747

Please make length same as title.

2762

Extra back-tick.

2854

Clang-tidy.

2857

Ditto.

2862

Ditto.

Eugene.Zelenko edited reviewers, added: alexfh, aaron.ballman, njames93, LegalizeAdulthood; removed: Restricted Project.May 23 2022, 3:39 PM
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
2742

Phabricator says there is no context available. Did you generate this diff with git diff -U999999 main?

2750

fall, not falling

2750

Below, not Bellow

2756

Does it have to be in the .clang-tidy file, or is it just a matter of setting options?

2772

Why is Struct listed twice?

2786

I would have expected ConstExprVariable instead?

2857

identifies, not identifiers

2873

parameters not paramaters

2873

Clang-tidy, not clang tidy

2880

Should we be linking to ephemeral gists that can disappear?

3054

This covers "const pointer", but what about "pointer to const"?
e.g. std::string const *str_ptr how is this handled?

3120

Same, pointer to const?

3127–3129

Is this really a bug report disguised as a doc comment?

KazNX updated this revision to Diff 432159.May 25 2022, 6:04 PM

Updated to address feedback.

  • Typo corrections.
  • Additional declarations.
  • Changed source reference URL.
whisperity retitled this revision from `readability-indentifier-naming` resolution order and examples to [clang-tidy][doc] Document readability-indentifier-naming resolution order and examples.Sun, Jun 19, 11:54 PM
whisperity added inline comments.Sun, Jun 19, 11:59 PM
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
2750–2752

So is this resolution order the same for every Decl?

2880

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.

KazNX marked 17 inline comments as done.Tue, Jun 21, 3:06 PM
KazNX added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
2742

No sorry. Will fix on next submission.

2750–2752

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.

2756

I've changed the wording.

2772

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

2786
2857

Intentionaly. I'm using the noun, not the verb.

2880

I'm changing this link to a full repo.

2880

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.

3054

I can add that. Empirically that turned out a ConstantParameter.

3054

I was wrong. There's no const association.

3120

I'm adding a few more variants including static.

3127–3129

Yes it is. Sorry, was captured from my original document. I agree it doesn't belong here.

3127–3129

I've added a new bug report for this: https://github.com/llvm/llvm-project/issues/55705

LegalizeAdulthood requested changes to this revision.Thu, Jun 23, 10:47 AM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Thu, Jun 23, 10:47 AM