Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting naming check with Hungarian notation.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
435–438 | Formatting | |
594–595 | ||
605–606 | ||
670 | ||
681–683 | No need to check for CXXMethodDecl because those inherit from FunctionDecl anyway. | |
697 | I think EOL should probably be const char * and can remove remove all these const_cast<>? | |
784 | I think this switch should be replaced with: if (const auto *ECD = dyn_cast<EnumConstantDecl>(ND)) { ... } else if (const auto *CRD = dyn_cast<CXXRecordDecl>(ND)) { ... } else if (isa<VarDecl, FieldDecl, RecordDecl>(ND)) { ... } | |
789 | Can remove all of the if (ND) in this method -- we already early return if ND == nullptr. | |
829–830 | ||
853 | ||
1017–1035 | ||
1325–1326 | There's no need to pass a StringRef by const reference -- they pass like a pointer type already. | |
1373 | And can remove the declaration of TypeName above. Also, can you correct the formatting issues, elsewhere as well. | |
clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp | ||
55 ↗ | (On Diff #306411) | This change looks unrelated to the patch? |
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp | ||
363 ↗ | (On Diff #306411) | Same here? |
clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp | ||
41 ↗ | (On Diff #306411) | And these changes as well? |
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst | ||
115 | For all of these, I'd recommend a slight wording tweak to: When enabled, the check ensures that the declared identifier will have a Hungarian notation prefix based on the declared type. | |
2521–2522 | I would remove this paragraph as it doesn't really add much value (it effectively says that the option works well with other options, but that's already expected). | |
2569 | ||
2572 | ||
2575 | ||
2578–2579 | ||
lldb/include/lldb/Target/Process.h | ||
565 ↗ | (On Diff #306411) | All the changes in this file are unrelated. |
lldb/source/Target/Process.cpp | ||
532 ↗ | (On Diff #306411) | Unrelated changes. |
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp | ||
167 ↗ | (On Diff #306411) | All the changes in this file are unrelated. |
llvm/lib/Target/SystemZ/SystemZISelLowering.h | ||
54 ↗ | (On Diff #306411) | Unrelated changes. |
llvm/lib/Target/SystemZ/SystemZInstrInfo.td | ||
835 ↗ | (On Diff #306411) | All the changes in this file are unrelated. |
llvm/lib/Target/SystemZ/SystemZInstrVector.td | ||
574 ↗ | (On Diff #306411) | Unrelated changes. |
llvm/lib/Target/SystemZ/SystemZOperators.td | ||
669 ↗ | (On Diff #306411) | All the changes in this file are unrelated. |
llvm/lib/Transforms/Scalar/ConstraintElimination.cpp | ||
15 ↗ | (On Diff #306411) | All the changes in this file are unrelated. |
Hi @aaron.ballman, thank you for your feedback. I will update my change later. Unrelated change were mixed with other commits when I against git master. I did it again then the problem was gone. I found the command, arc diff master --preview, knowing exactly changes before uploading diff by arc.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp | ||
---|---|---|
255 | I improved it. It will look like the following: static constexpr std::pair<StringRef, StringRef> CStrings[] = { {"char*", "sz"}, {"char[]", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t[]", "wsz"}}; | |
377–381 | Improved here too. It will change to: static constexpr std::pair<StringRef, StringRef> HNCStrings[] = { {"CharPrinter", "char*"}, {"CharArray", "char[]"}, {"WideCharPrinter", "wchar_t*"}, {"WideCharArray", "wchar_t[]"}}; | |
435–438 | OK. I checked all the range including single-line if statements, and passing StringRef directly(not its reference). |
- 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 | ||
---|---|---|
620 | 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 | ||
---|---|---|
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. | |
446–473 |
- 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.