[clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions

[njames93]

Summary

Typically most main functions have the signature:

int main(int argc, char *argv[])

To stick with convention when renaming parameters we should ignore the argc and argv names even if the parameter style says they should be renamed. This patch addresses this by checking all ParmVarDecls if they form part of a function with a signature that matches main int name(int argc, char * argv[], (optional char *env[]))

[njames93]

For the record this convention appears everywhere in LLVM even though LLVM style is to have parameters starting with uppercase letters

[merge_guards_bot]

Unit tests: pass. 62058 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 62058 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[Mordante]

Would it make sense to also allow wmain with wchar_t? https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019

[aaron.ballman]

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

498

I think a better approach may be to look at the DeclContext for the ParmVarDecl object to see if it is a FunctionDecl, and if it is, call FunctionDecl::isMain() to check.

[njames93]

In D73098#1832131, @Mordante wrote:

Would it make sense to also allow wmain with wchar_t? https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019

Does wmain get used in a lot of projects. If it's very niche then I don't feel it warrants a place in here. If it does I'll add it in.

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

498

I specifically didn't want to do that as is I want to get functions that act like main, usually the case when main itself dispatches to other functions with the same signature.

[aaron.ballman]

In D73098#1832489, @njames93 wrote:

In D73098#1832131, @Mordante wrote:

Would it make sense to also allow wmain with wchar_t? https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019

Does wmain get used in a lot of projects. If it's very niche then I don't feel it warrants a place in here. If it does I'll add it in.

It's not uncommon on Windows, at the very least. I think it's worth supporting.

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

498

I'm not certain that's a case we should support -- users who write their own main-like interfaces should probably follow their usual naming rules (even if we don't do it ourselves). For instance, this will catch functions that are definitely *not* main-like except in the signature. e.g., int accumulate_string_lengths(int count, char *strings[]);

[njames93]

In D73098#1833736, @aaron.ballman wrote:

In D73098#1832489, @njames93 wrote:

In D73098#1832131, @Mordante wrote:

Would it make sense to also allow wmain with wchar_t? https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019

Does wmain get used in a lot of projects. If it's very niche then I don't feel it warrants a place in here. If it does I'll add it in.

It's not uncommon on Windows, at the very least. I think it's worth supporting.

I'll add that in

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

498

Thats a good point, how about checking if the name of the function starts or ends with "main" or "wmain" for windows, maybe even have an option to enable ignore "main like" functions, but always ignore the actual main function

[njames93]

• Address nits

[merge_guards_bot]

Unit tests: pass. 62115 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 1 errors and 0 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 62133 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• use regex for func name matching

[njames93]

• Dont trim func name before regex

[merge_guards_bot]

Unit tests: pass. 62131 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 62131 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

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

380

Any thoughts on whether I should detect WMain or just Wmain. WMain looks more pleasing but using CamelCase rules it should likely be Wmain

[aaron.ballman]

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

380

I think this needs to be controlled by an option if we want to keep it, because you can still have names like terminateMain(int exitCode, char *msgs[]); At the end of the day, no matter what regex we come up with, we can probably pick an identifier that subverts the check. Also, the name main might have different meanings if the function is a member function or a member of a namespace (or, alternatively, it may actually relate to the main entrypoint). Ultimately, this seems too much like guessing at user intent, which is why I think it should be an option (and probably off by default).

That said, if we're searching for main-like names, I think we need to find names like wmain, Wmain, wMain, WMain optionally with a prefix or suffix (perhaps with underscores). I am less certain how to treat member functions, but suspect the reasonable thing to do is treat them as being potentially main-like regardless of whether they're static or not, but the function still needs to be publicly available rather than private or protected (those can't be main-like, unless someone friends main... which is another twist).

[njames93]

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

380

Its current behaviour now is to just flag main, but there is an option IgnoreMainLikeFunctions(defaults to off) that if turned on will look for functions with the right sig that have the word main, Main, WMain, Wmain or wmain in there. I'll add in the public access though, that was an oversight

[njames93]

• Added checks for access specifiers

[merge_guards_bot]

Unit tests: pass. 62135 tests passed, 0 failed and 812 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62153 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[aaron.ballman]

LGTM aside from some minor nits.

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

337

I'd flip the logic to != AS_public to be more clear that we only care about public members.

clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst

833

We should document that this defaults to 0 and mention wmain as well.

[njames93]

• Address nits

[merge_guards_bot]

Unit tests: pass. 62196 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

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

337

What do you think about AS_none, its a weird case and shall i ignore it. I'm guessing all functions not in a class are implicitly declared to be public.

[aaron.ballman]

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

337

Good catch!

[njames93]

May need to roll this back, I think as i had this on the same local branch as D73052 I have brought both of them in at once

[njames93]

Fix diff, ready to recommit