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

[dougpuob]

Summary

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

[aaron.ballman]

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]

Against git master.

[dougpuob]

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]

• 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]

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]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

620

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

[dougpuob]

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

[dougpuob]

• 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]

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

[njames93]

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]

• 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]

arc diff with the correct base.

[njames93]

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]

• Against git master.
• Refined isHungarianNotationOptionEnabled() function.
• Classify document and test cases.

[dougpuob]

• Fixed assertions by clang-format.

[dougpuob]

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

[njames93]

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

[dougpuob]

In D86671#2456161, @njames93 wrote:

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]

• 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]

• Removed unrelated comments.

[dougpuob]

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.

[dougpuob]

Ping @njames93