Page MenuHomePhabricator

[clang-tidy] Add new case type to check variables with Hungarian notation
AcceptedPublic

Authored by dougpuob on Aug 26 2020, 7:00 PM.

Details

Summary

Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting naming check with Hungarian notation.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Nov 19 2020, 10:48 AM
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.

dougpuob updated this revision to Diff 306867.Nov 21 2020, 11:57 AM

Against git master.

dougpuob marked 82 inline comments as done.Nov 22 2020, 3:43 AM

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).

dougpuob removed a project: Restricted Project.Nov 22 2020, 5:00 AM
dougpuob updated this revision to Diff 306908.Nov 22 2020, 5:37 AM
dougpuob marked 3 inline comments as done.
  • 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).
aaron.ballman added inline comments.Nov 23 2020, 6:06 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
622–625
677–678
715

This comment doesn't appear to have been handled.

746

Similar to what was suggested above.

Eugene.Zelenko added inline comments.Nov 23 2020, 6:41 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
620

Returning {} as default initializer will be better. Same in other places.

dougpuob marked 5 inline comments as done.Nov 24 2020, 7:56 AM

Hi @aaron.ballman and @Eugene.Zelenko, thank you for your suggestions. I will improve them and upload my diff later.

dougpuob updated this revision to Diff 307384.Nov 24 2020, 9:20 AM
  • 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>.
aaron.ballman accepted this revision.Dec 4 2020, 8:45 AM

LGTM! Thank you for the new functionality! You may want to wait a bit before landing to see if @njames93 has any remaining comments.

This revision is now accepted and ready to land.Dec 4 2020, 8:45 AM
njames93 added inline comments.Dec 4 2020, 3:23 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
441–445
450–454
462–466
514–518
553–554
572
579
592
598–607
613–615
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
138–139

This should be part of the FileStyle struct.

dougpuob marked 11 inline comments as done.Dec 6 2020, 6:45 AM
dougpuob updated this revision to Diff 309767.Dec 6 2020, 6:48 AM
  • Improved code review suggestions from @njames93. Including move the IdentifierNamingCheck::HNOption variable to IdentifierNamingCheck::FileStyle::HNOption, use try_emplace() instead of insert() and lookup().
dougpuob updated this revision to Diff 309770.Dec 6 2020, 7:01 AM

arc diff with the correct base.

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.

dougpuob updated this revision to Diff 311211.Dec 11 2020, 6:23 AM
  • Against git master.
  • Refined isHungarianNotationOptionEnabled() function.
  • Classify document and test cases.
dougpuob updated this revision to Diff 311267.Dec 11 2020, 9:59 AM
  • Fixed assertions by clang-format.
dougpuob marked an inline comment as done.Dec 11 2020, 10:25 PM

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

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?

Hi @njames93, it's a good idea. I am trying to do it, thank you.

dougpuob updated this revision to Diff 313970.Dec 29 2020, 7:50 AM
  • 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.
dougpuob updated this revision to Diff 314076.Dec 29 2020, 11:01 PM
dougpuob marked 2 inline comments as done.
  • Removed unrelated comments.

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.