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

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

245–251

OK. I checked all the range including single-line if statements, and passing StringRef directly(not its reference).

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[]"}};

[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

920–923

975–976

1013

This comment doesn't appear to have been handled.

1044

Similar to what was suggested above.

[Eugene.Zelenko]

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

918

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

251–255

260–264

272–276

324–328

363–364

382

389

402

408–417

423–425

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

207–208

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

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.

[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

[dougpuob]

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

[njames93]

Sorry, I thought this landed months ago. I'll take a proper look again next week.

clang-tools-extra/clang-tidy/ClangTidyCheck.h

158

This isn't really needed, you can just call configurationDiag directly.

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

263

[dougpuob]

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;

    ...
}

[njames93]

clang-tools-extra/clang-tidy/ClangTidyCheck.h

124–126

You don't need a ClangTidyContext, this function is all that's needed.

[dougpuob]

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.

[dougpuob]

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

[dougpuob]

• Removed diagnoseInvalidConfigOption() function.
• Created a HungarianNotation class and moved related functions into there.

[dougpuob]

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.

[njames93]

In D86671#2750957, @dougpuob wrote:

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.

[dougpuob]

In D86671#2754542, @njames93 wrote:

In D86671#2750957, @dougpuob wrote:

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.

Thank you for your reply and suggestion in code, I will try it.

[dougpuob]

• Moved all static functions into IdentifierNamingCheck class, and called the configurationDiag() directly in IdentifierNamingCheck::getFileStyleFromOptions() method.

[dougpuob]

@njames93, Ping