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

[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

[• gnossier]

Is this ready to merge soon?

[dougpuob]

In D86671#2837818, @gnossier wrote:

Is this ready to merge soon?

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?

[thakis]

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?

[dougpuob]

In D86671#2919077, @thakis wrote:

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?

[thakis]

In D86671#2920294, @dougpuob wrote:

In D86671#2919077, @thakis wrote:

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?

Yes :)

[aeubanks]

you can reopen the revision via "Add Action..."

[thakis]

Any news here? This has been broken for over a day now.

[dougpuob]

In D86671#2920570, @aeubanks wrote:

you can reopen the revision via "Add Action..."

Hi @aeubanks , thank you for the suggestion.

[dougpuob]

In D86671#2921424, @thakis wrote:

Any news here? This has been broken for over a day now.

Hi @thakis,
YES. I just create a new differential diff for it, https://reviews.llvm.org/D107325, could you help me to review it?