Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting naming check with Hungarian notation.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
- Improved code review suggestions from Aaron Ballman(aaron.ballman). Including document, single-line if statements, no reference with StringRef, "char"-->"char[]" for HNOption.CString, use llvm::transform insteads of for-loop to uppercase string, don't check CXXMethod in getDeclTypeName(), remove const_cast<char *> in getDeclTypeName(), use if-else insteads of switch-case(simplify src).
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
918 | Returning {} as default initializer will be better. Same in other places. |
Hi @aaron.ballman and @Eugene.Zelenko, thank you for your suggestions. I will improve them and upload my diff later.
- Improved code review suggestions from Aaron.Ballman and Eugene.Zelenko. Including return default initializer instead of empty string, use isa<>() instead of if statement with multiple conditions, use StringRef array instead of list<std::string>.
LGTM! Thank you for the new functionality! You may want to wait a bit before landing to see if @njames93 has any remaining comments.
- Improved code review suggestions from @njames93. Including move the IdentifierNamingCheck::HNOption variable to IdentifierNamingCheck::FileStyle::HNOption, use try_emplace() instead of insert() and lookup().
This needs rebasing against main. Can't be applied cleanly in its current state.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
399–405 | There's no need to build a string and transform it here. |
- Against git master.
- Refined isHungarianNotationOptionEnabled() function.
- Classify document and test cases.
Hi @njames93, thank you for your review suggestions, I have improved them and against my change to the main branch.
I encounter a problem on the Buildbot for Windows only. Several people encounter also to the same problem that their build failed at an unrelated regression test (llvm\test\CodeGen\XCore\threads.ll).
One last point, is there a way to validate that these options are only set where it makes sense. If someone tries to use say MacroDefinitionHungarianPrefix That could be warning worthy?
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
259–286 | ||
399–405 | Scratch this, in D92756 support was added for all boolean options in clang-tidy to use the full YAML boolean spec, I'd advise calling the same function here to keep everything consistent. |
- Against git master.
- Refined isHungarianNotationOptionEnabled() function.
- Classify document and test cases.
- Fixed assertions by clang-format.
- Instead check On/Off string with llvm::yaml::parseBool() function for consistent.
- Improved code.
- Add new check for unsupported option.
Addressed comments by @njames93. Including adding warning message for unsupported options in config file, refining code in getFileStyleFromOptions(), and for consistent reason to use llvm::yaml::parseBool() function instead of checking On/Off string.
- Against trunk.
- Refined isHungarianNotationOptionEnabled() function.
- Changed old way that showing invalid config option in .clang-tidy config file. Now added a new function ClangTidyCheck::OptionsView::diagnoseInvalidConfigOption() instead.
- Recovered that document content missing in .rst.
Hi @njames93,
That's ok. You have helped me a lots. Thank you.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
263 | To use configurationDiag() function in getFileStyleFromOptions(), seems it needs to pass a ClangTidyContext object to getFileStyleFromOptions(). How about changing like the following? getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options, ClangTidyContext &Context) { ... if (!isHungarianNotationSupportedStyle(I) && HPTOpt.hasValue()) Context.configurationDiag("invalid identifier naming option '%0'") << StyleString; ... } |
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
---|---|---|
124–126 | You don't need a ClangTidyContext, this function is all that's needed. |
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
---|---|---|
124–126 | But the getFileStyleFromOptions() is a static function in IdentifierNamingCheck.cpp file. It can't access configurationDiag() directly. |
Hi @njames93
I tried to create a class, HungarianNotation, and put all related functions into the class. Then I can call configurationDiag() in HungarianNotation::checkOptionValid() function.
How about this way?
The new class.
struct HungarianNotation { public: HungarianNotation(ClangTidyContext *Context = nullptr); bool checkOptionValid(int StyleKindIndex, StringRef StyleString, bool HasValue); ...
Use the configurationDiag() in checkOptionValid() function
bool HungarianNotation::checkOptionValid(int StyleKindIndex, StringRef StyleString, bool HasValue) { .. if (HasValue) Context->configurationDiag("invalid identifier naming option '%0'") << StyleString; return false; }
Call the checkOptionValid() here in getFileStyleFromOptions().
static IdentifierNamingCheck::FileStyle getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options, ClangTidyContext &Context) { ... StyleString.append("HungarianPrefix"); auto HPTOpt = Options.get<IdentifierNamingCheck::HungarianPrefixType>(StyleString); HN.checkOptionValid(I, StyleString, HPTOpt.hasValue()); ...
- Removed diagnoseInvalidConfigOption() function.
- Created a HungarianNotation class and moved related functions into there.
Hi @njames93:
Could you do me a favor? Because it is my first patch, something I'm not sure. I'm confused about can I land this patch now? I read the "LLVM Code-Review Policy and Practices" document, it said patches can be landed if received a LGTM, but seems you are still reviewing.
If you have made significant changes (excluding what a reviewer asks when giving an LGTM) Its best to get those further changes also reviewed.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
246 | Rather than passing the ClangTidyContext, Make this function a method of IdentifierNamingCheck. Then you call create the Diag without worrying about the Context. |
- Moved all static functions into IdentifierNamingCheck class, and called the configurationDiag() directly in IdentifierNamingCheck::getFileStyleFromOptions() method.
Hi @gnossier:
I'm waiting for feedbacks from reviewer.
Hi @aaron.ballman:
Nathan is helping me to review this patch, but seems he is not here recently. Do you have any suggestion about how to keep the ball rolling in this situation? If it was done, can I apply the right to land it by self?
This breaks tests on windows: http://45.33.8.238/win/43180/step_8.txt
"The command line is too long" Maybe some arts could be passed via a response file instead?
Hi @thakis :
Thank you.
I'm a newbie. I tried to arc diff but arc shows ERR_CLOSED: This revision has already been closed.. Should I create a new differential diff for it?
Hi @thakis,
YES. I just create a new differential diff for it, https://reviews.llvm.org/D107325, could you help me to review it?