Page MenuHomePhabricator

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

Authored by njames93 on Jan 21 2020, 6:03 AM.

Details

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

Diff Detail

Event Timeline

njames93 created this revision.Jan 21 2020, 6:03 AM

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

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

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

aaron.ballman added inline comments.Jan 21 2020, 2:03 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
503

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 marked an inline comment as done.Jan 21 2020, 2:34 PM

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
503

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.

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
503

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 marked an inline comment as done.Jan 22 2020, 8:19 AM

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
503

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 updated this revision to Diff 239734.Jan 22 2020, 4:28 PM
  • Address nits

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

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 updated this revision to Diff 239825.Jan 23 2020, 2:40 AM
  • use regex for func name matching
njames93 updated this revision to Diff 239830.Jan 23 2020, 2:45 AM
  • Dont trim func name before regex
njames93 marked 2 inline comments as done.Jan 23 2020, 2:45 AM

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

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 added inline comments.Jan 23 2020, 3:14 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
383

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 added inline comments.Jan 23 2020, 5:33 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
383

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 marked an inline comment as done.Jan 23 2020, 6:40 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
383

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 updated this revision to Diff 239902.Jan 23 2020, 7:31 AM
  • Added checks for access specifiers

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.

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 accepted this revision.Jan 25 2020, 11:37 AM

LGTM aside from some minor nits.

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

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.

This revision is now accepted and ready to land.Jan 25 2020, 11:37 AM
njames93 marked 4 inline comments as done.Jan 25 2020, 4:14 PM
njames93 updated this revision to Diff 240400.Jan 25 2020, 4:15 PM
  • Address nits

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 marked an inline comment as done.Jan 27 2020, 6:14 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
340

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 accepted this revision.Jan 27 2020, 6:54 AM
aaron.ballman added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
340

Good catch!

This revision was automatically updated to reflect the committed changes.

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 updated this revision to Diff 240706.Jan 27 2020, 3:44 PM

Fix diff, ready to recommit

njames93 reopened this revision.Jan 27 2020, 3:45 PM
This revision is now accepted and ready to land.Jan 27 2020, 3:45 PM
This revision was automatically updated to reflect the committed changes.