Before this patch, readability-identifier-naming contained a significant amount of logic for (a) checking the style of identifiers, followed by (b) renaming/applying fix-its. This patch factors out (b) into a separate base class so that it can be reused by other checks that want to do renaming. This also cleans up readability-identifier-naming significantly, since now it only needs to be concerned with the interesting details of (a).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
352 | Please elide braces. Same below. |
I guess all the tests run?
I think this LGTM after the nits are adressed.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
204 | What about the else? If it is unreachable it should have a llvm_unreachable("Reason");, if not, i think a little fallthrough comment wouldn't hurt. | |
260 | Please add punctuation here and in the comments below. |
Addressed some nits. If the rest looks good, I'll need someone with commit access to help me wrap this up.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
273 | Please elide braces. | |
281 | Please elide braces. | |
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h | ||
48 | It'll be reasonable to place all comments consistently: before definition or after. | |
95 | Please use using. Same below. See modernize-use-using. |
More nits.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
273 | Would you mind pointing me toward a resource for these formatting nits? I don't see anything about requiring omitting braces for single-statement if()s in the official LLVM coding standards (and I happen to think the braces actually help readability here). If this stuff is explicitly documented somewhere and painstakingly enforced like this, I'd rather get it right the first time. |
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
273 | I looked through the coding standard and it does not suggest this, which I am really surprised to see (I feel like the standard used to say it at some point, but it likely got removed and I did not notice). This is a common pattern that's used all over the code base (likely from fairly consistent review guidance over the years), and so it's best to be consistent with the style used locally, but that's a pretty weak argument for new code. |
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
273 | Did it maybe get dropped? Wasn't there are overhaul a few weeks ago? But I am surprised as well, as this was at one point explicitly stated. |
i am still fine with the patch. if no one else has objections, i think this can land.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp | ||
---|---|---|
273 | I went as far back as the 4.0 coding standards and it wasn't listed in there, but I'm with you on remembering this being explicitly stated somewhere. |
(Just in case this got lost in the shuffle)
If this looks good, (I think) I'll need someone with commit access to help wrap it up.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h | ||
---|---|---|
30 | Should this be protected as this class should never be instantiated directly. Also the definition could be moved inline as its just a delegating constructor |
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h | ||
---|---|---|
30 | The constructor (and destructor) can't be inline, since they need to be able to see the specialization of DenseMapInfo<NamingCheckId> in the cpp. I could change it to protected to clean up the interface -- though it won't strictly change anything, since the class already has pure virtual functions so it's not instantiable. |
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h | ||
---|---|---|
30 | Searched the LLVM codebase for other abstract base classes with explicitly defined constructors (e.g. MakeSmartPtrCheck in clang-tidy), and their constructors seem to be public. I think I'll keep this one public too for consistency. |
Thank you for the patch, I've commit on your behalf in d5c6b8407c12d39a78f42a216369407cb2d7b511
Should this be protected as this class should never be instantiated directly. Also the definition could be moved inline as its just a delegating constructor