In D148314#4268989, @PiotrZSL wrote:

I will push into repository this in a moment....

In D148314#4268460, @PiotrZSL wrote:

Change in tests is ok, change in release notes not needed (please remove).
Change title into [clang-tidy][NFC] Improved ...

NFC - Non Functional Change

• Removed unnecessary information from ReleaseNotes.rst

In D148314#4267567, @PiotrZSL wrote:

And you forget to attach changes in unit tests.

Against trunk.

In D147779#4257212, @PiotrZSL wrote:

arc land should work, I usually use arc patch --nobranch to check things before committing and then git push.

Hi @PiotrZSL
I do not have permission to land the change. Can you please help me to land this change?

In D144510#4256258, @amurzeau wrote:

In D144510#4254959, @dougpuob wrote:

Hi @amurzeau

I missed the review before landing. I have a suggestion regarding the customized prefix "cust" because it confused me when I found the Hungarian notation patterns in the output message. Perhaps using "my" instead of "cust" would improve readability.

Take some examples:

• const char *custszNamePtr = "Name"; --> myszNamePtr
• uint8_t custu8ValueU8 = 0; --> myu8ValueU8
• DWORD custdwMsDword = 0; --> mydwMsDword

Hi,

Yes this is a good idea, m and y are less used as type prefixes.

Do you want to make the change ? I can do it if you want.

I missed the review before landing. I have a suggestion regarding the customized prefix "cust" because it confused me when I found the Hungarian notation patterns in the output message. Perhaps using "my" instead of "cust" would improve readability.

• Improved suggestions in code review

• Extract method
• Remove template parameters in the getDeclTypeName() function

In D147779#4251081, @PiotrZSL wrote:

Missing release notes.

About the error message, "relocation truncated to fit: R_68K_PC16 against '.rodata.str1.1'" when compile with -static option. I traced the source code. M68K doesn't support CodeModel::Large and CodeModel::Kernel. So, to build a static executable binary via clang, adding -mcmodel=medium is a suitable choice at this moment.

In D116886#3246619, @0x59616e wrote:

In D116886#3246562, @dougpuob wrote:

In D116886#3242633, @0x59616e wrote:

In D116886#3242621, @myhsu wrote:

In D116886#3240566, @0x59616e wrote:

In D116886#3229948, @dougpuob wrote:

Hi @myhsu
Even though the instruction selection is focused on sub 0 x only, I still want to build a simple function with clang and run it with QEMU user emulator (qemu-m68k). But I have no idea how to cross-compile it with GCC runtime, do you have any document for it or could you show me a brief instruction.

FYI, You can build it with
clang --target=m68k $(SOURCE_FILE) -c -o
and then
m68k-linux-gnu-gcc $(SOURCE_OBJECT) -o $(FILE_NAME)

or
clang --target=m68k $(SOURCE_FILE) -o $(FILE_NAME) -ccc-gcc-name linux-gnu-gcc

Currently lld doesn't have M68k support, but I'm trying on it.
In the future, you may use lld to link m68k object with:
`clang --target=m68k $(SOURCE_FILE) -o $(FILE_NAME) -fuse-ld=lld

LLD is not required, you can use m68k-linux-gnu-ld to link. The key is using the correct target triple: m68k-linux-gnu rather than just m68k. Such that the compiler driver can find the right tools (in this case m68k-linux-gnu-ld) and libraries to use.
Actually, I just put up a document regarding m68k cross-compiling: https://m680x0.github.io/doc/cross-compile-how-to

That makes sense. Thanks

Hi @0x59616e and @myhsu,
Thank your reply. Now I can compile a hello program as an elf executable binary for m68k and execute it in Debian (qemu-system-m68k).

But I still cannot build the hello program with -static. My sample program just call print("Hello M68K!!! \n") in main(). Is it an issue?

❯ bin/clang++ --target=m68k-linux-gnu -static hello.cpp -o hello.m68k.out
/tmp/hello-a7168e.o: in function `main':
hello.cpp:(.text+0x14): relocation truncated to fit: R_68K_PC16 against `.rodata.str1.1'
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

Hi,

I haven't dived into the problem, so this just my guess.

It seems that the address of the string (.rodata.str.1, I guess it's the string in your printf) is too far from the instruction itself, PC16 addressing mode is not enough, it needs (maybe ?) PC32.

I guess this issue may have something to do with the code model ?

• Rebase against trunk.

• Add a descriptive comment.

In D116886#3242633, @0x59616e wrote:

In D116886#3242621, @myhsu wrote:

In D116886#3240566, @0x59616e wrote:

In D116886#3229948, @dougpuob wrote:

Hi @myhsu
Even though the instruction selection is focused on sub 0 x only, I still want to build a simple function with clang and run it with QEMU user emulator (qemu-m68k). But I have no idea how to cross-compile it with GCC runtime, do you have any document for it or could you show me a brief instruction.

FYI, You can build it with
clang --target=m68k $(SOURCE_FILE) -c -o
and then
m68k-linux-gnu-gcc $(SOURCE_OBJECT) -o $(FILE_NAME)

or
clang --target=m68k $(SOURCE_FILE) -o $(FILE_NAME) -ccc-gcc-name linux-gnu-gcc

Currently lld doesn't have M68k support, but I'm trying on it.
In the future, you may use lld to link m68k object with:
`clang --target=m68k $(SOURCE_FILE) -o $(FILE_NAME) -fuse-ld=lld

LLD is not required, you can use m68k-linux-gnu-ld to link. The key is using the correct target triple: m68k-linux-gnu rather than just m68k. Such that the compiler driver can find the right tools (in this case m68k-linux-gnu-ld) and libraries to use.
Actually, I just put up a document regarding m68k cross-compiling: https://m680x0.github.io/doc/cross-compile-how-to

That makes sense. Thanks

• Add a break to the case of switch statement. (Suggestions from RKSimon)

• Refine the code and increase test coverage. (Suggestions from RKSimon)

• Remove braces for single line if statements.

Hi @myhsu
Even though the instruction selection is focused on sub 0 x only, I still want to build a simple function with clang and run it with QEMU user emulator (qemu-m68k). But I have no idea how to cross-compile it with GCC runtime, do you have any document for it or could you show me a brief instruction.

• Apply only the dividend value is zero and revert *.ll files.

In D107325#2923518, @thakis wrote:

The bot is happy again, thanks.

Hi @thakis:
I can't access the repo, could you please help me to land?

• Improved code review suggestions.

In D107325#2921844, @thakis wrote:

Thanks!

Since the config file contents are fixed as far as I can tell, maybe we could instead just add a file with the right contents to clang-tools-extra/test/clang-tidy/checkers/Inputs and refer to that? (Use %S/Inputs/myfile.txt) (Alternatively you could use the split-file utility, but putting the file in Inputs is probably easier if you're new, and it's not any worse :) )

• Moved config content of regression test to clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/hungarian-notation*/.clang-tidy, then refer to those files in tests.

In D86671#2921424, @thakis wrote:

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

In D86671#2920570, @aeubanks wrote:

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

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?

In D86671#2837818, @gnossier wrote:

Is this ready to merge soon?

@njames93, Ping

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

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.

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.

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

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?

That's ok. You have helped me a lots. Thank you.

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

Ping @njames93

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.

• Removed unrelated comments.

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

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, thank you for your review suggestions, I have improved them and against my change to the main branch.

• Fixed assertions by clang-format.

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

arc diff with the correct base.

• Improved code review suggestions from @njames93. Including move the IdentifierNamingCheck::HNOption variable to IdentifierNamingCheck::FileStyle::HNOption, use try_emplace() instead of insert() and lookup().

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

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

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.

Against git master.

Against git master.

ping?

• Shortened namespace with type alias.
• Removed namespace for StringRef.

• Fixed warnings of Clang-Tidy.

1. Fixed build failure on BuildBot. (Touch empty data from Options.get())
2. Changed llvm::StringMap with std::pair in getHungarianNotationDefaultConfig().
3. Moved HNOpts and HNDerviedTypes declarations out of getHungarianNotationDefaultConfig().
4. Reused SmallString<128> for string concatenation.
5. Removed redundant m(HungarianNotation) in #define NAMING_KEYS(m).

1. Applied all patches with the trunk(1800b44651c19b11e7680f908344d5751e8d2246).
2. Moved HNOption pointer out of NamingStyle data structure.
3. Removed parseHungarianPrefix() function, use Options.get() instead.

• Removed clearAll() function and made the HPOption variable passing by value(origin is std::move()).
• Moved static HNOption variable from static getNamingStyles() function to IdentifierNamingCheck class a private data member.
• Replaced auto keyword with explicit std::string.
• Elided braces around single-line.
• Used CXXRecordDecl::isAbstract() directly.
• changed const auto to const auto * for pointer objects.
• Enhanced test case for enum with two EnumConstant decls.
• Removed unnecessary clang:: namespace for variables.
• Executed arc lint
• Final changes.
• Executed arc lint

Reply code review suggestions. I will upload my change later.

• Changed the type of HNOption from std::shared_ptr to reference.

Thank you for the suggestion.

• Changed Hungarian Notation as an prefix style (not a case type).
• Changed XxxHungarianPrefix as an enumeration from bool.
• Updated documents.

In D86671#2342016, @njames93 wrote:

In D86671#2341838, @dougpuob wrote:

Hi @njames93,

It's a smart idea, the rework for it is worth. There is a special case if lowercase name with Hungarian prefix, it possibly makes variable ambiguous, like the Case1. How about separating them and aNy_CasE with an underscore, like Case2 ?

// Case1
bool bRIGHT_LEVEL;     // UPPER_CASE
bool bRightLevel;      // CamelCase
bool bRight_Level;     // Camel_Snake_Case
bool baNy_CasE;        // aNy_CasE
bool bright_level;     // lower_case
bool brightLevel;      // camelBack
bool bright_Level;     // camel_Snake_Back
.....^^^^^^ <-- right? bright?

// Case2
bool bRIGHT_LEVEL;     // UPPER_CASE
bool bRightLevel;      // CamelCase
bool bRight_Level;     // Camel_Snake_Case
bool b_aNy_CasE;       // aNy_CasE
bool b_right_level;    // lower_case
bool b_rightLevel;     // camelBack
bool b_right_Level;    // camel_Snake_Back
.....^^^^^^^ <-- add an underscore

That still has hidden surprises. Maybe instead of a bool, an enum is used for controlling hungarian prefix (Off|On|...).
Can't think of a good name for the third option but it would do the inserting of '_' (bright_level ->b_right_level) or capitalising the first word of the identifier (brightLevel -> bRightLevel).

In D86671#2330689, @njames93 wrote:

Is this diff been created incorrectly again?

Taking a step back, Is Hungarian notation really a case style, Seems to me its mainly about the prefix and a user may want DWORD dwUPPER_CASE, Right now there is no way of adopting that.
Maybe extend the options for hungarian type decls to

<Type>Case
<Type>Prefix
<Type>Suffix
<Type>HungarianPrefix

<Type>HungarianPrefix would likely be a bool and if enabled, it would be prepended to <Type>Prefix
I could see a situation like this

[Options]
// VariableCase: UPPER_CASE
// VariablePrefix: PRE_
// VariableSuffix: _POST
// VariableHungarianPrefix: true

DWORD MyVariable; -> DWORD dwPRE_MY_VARIABLE_POST;

This would give users full control of exactly how they want their declarations with no hidden surprises.

Granted this approach would require a little rework but it would satisfy more users.

Merged all commits as one single commit. (Trying to resolve "No such file or directory" error from the scripts/phabtalk/apply_patch2.py!_apply_diff() function)

Hi @aaron.ballman and @njames93,
I addressed your code review suggestions and supported Hungarian Notation prefix for decl of enum and class(option) at latest patch. Unfortunately, I encountered a problem that new patch failed on remote BuildBot, it showed the "No such file or directory" error message when it was trying to call apply_patch2.py!_apply_diff(). Do you have any idea what is going on? Do you suggest I create a new Diff(new diff id) for it ?

Rename file name, "readability-identifier-naming-hungarian-notation-default.cpp" to "readability-identifier-naming-hungarian-notation.cpp".

Merged with the master then do arc diff master --update D86671 again. (Trying to resolve "No such file or directory" error from the scripts/phabtalk/apply_patch2.py!_apply_diff() function)

Previous arc diff got problem(failed on builtbot), do it again with arc diff master --update D86671.

• Support to add Class prefix for Hungarian Notation.
• Support to add Enum prefix for Hungarian Notation.
• Support unsigned long long, ULONG, and HANDLE types and others.
• Support options for Hungarian Notation in config file.
• Added more test cases.

In D86671#2304337, @njames93 wrote:

Not strictly relevant here, but this does open up the idea of enforcing the style where an enum constant is prefixed by the initials of the enum name.

In D86671#2293128, @aaron.ballman wrote:

In D86671#2284078, @dougpuob wrote:

Hi @aaron.ballman
About changing size_t nLength to cbLength. I searched MSVC folders with size_t, many names of variable start with n, or i in MFC related files. So I prefer to keep it starts with n. Another side to name starts with cb, I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR type.

I think the important point is that cb is used for APIs that specify a count of bytes, whereas i, n, and l are used for integers (there is no specific prefix for size_t that I'm aware of or that MSDN documents, so the lack of consistency there is not too surprising). That's why I mentioned that using a heuristic approach may allow you to identify the parameters that are intended to be a count of bytes so that you can use the more specific prefix if there's more specific contextual information available.

Hi @aaron.ballman
About changing size_t nLength to cbLength. I searched MSVC folders with size_t, many names of variable start with n, or i in MFC related files. So I prefer to keep it starts with n. Another side to name starts with cb, I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR type.

Replied comments by @aaron.ballman

In D86671#2259492, @njames93 wrote:

In D86671#2259443, @dougpuob wrote:

In D86671#2259364, @njames93 wrote:

Did you upload this incorrectly again, context is missing and seems to be a relative diff from a previous version of this patch?

Sorry for it, I think it's my bad. It is possible that I manually merged the last master(github) with changes then updated them both via web interface ...

Can I fix it if switch back to the base (14948a0) then merge all my changes, then update the diff again via web interface? Or do you have any better suggestion?

I am curious about how do you know this mistake? You got error messages with arc patch D86671 ?

The no context is easy to spot as phab says context not available. Its easy to find knowing that there is no mention of hungarian notation in the trunk version of IdentifierNamingCheck.cpp, yet there is mention of that in the before diff.

The way I do my patches is I create a branch from the current master. Then all commits go into that branch. When its time to update the PR I can just do a diff from <feature_branch> to <master>.
Though I do use arcanist for my patches

arc diff master

arcanist will check to see if the current branch has tags for PR and automatically update that PR. Otherwise it will create a new PR.
If it goes to create a new PR instead of updating an existing one you can pass update

arc diff master --update D86671

Fixed crash on Windows when run regression test (llvm-lit for readability-identifier-naming.cpp file).

• Fixed lint warnings and regression test failures on Windows.