Page MenuHomePhabricator

readability check for const params in declarations
ClosedPublic

Authored by fowles on Mar 23 2016, 10:57 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

fowles updated this revision to Diff 51446.Mar 23 2016, 10:57 AM
fowles retitled this revision from to readability check for const params in declarations.
fowles updated this object.
fowles added a reviewer: alexfh.
fowles added a subscriber: cfe-commits.
alexfh edited edge metadata.Mar 24 2016, 7:41 AM

Looks good in general. A few nits.

clang-tidy/readability/AvoidConstParamsInDecls.cpp
22 ↗(On Diff #51446)

"Function names ... should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())."
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

50 ↗(On Diff #51446)

nit: The Diag name is more common for this purpose.

51–53 ↗(On Diff #51446)

This is the best wording I could come up with in an internal review, but it's still not ideal. If someone knows how to improve this, ideas are welcome.

docs/clang-tidy/checks/readability-avoid-const-params-in-decls.rst
4 ↗(On Diff #51446)

To many '='s.

8 ↗(On Diff #51446)

Either "do not have any effect on ..." or "do not affect ..."

15 ↗(On Diff #51446)

Please use proper capitalization and punctuation in the comments.

fowles updated this revision to Diff 51596.Mar 24 2016, 1:32 PM
fowles marked 5 inline comments as done.
fowles edited edge metadata.
  • review comments
alexfh accepted this revision.Mar 29 2016, 9:42 AM
alexfh edited edge metadata.

Looks good with one nit. Thank you for the new check!

Do you need me to submit the patch for you?

clang-tidy/readability/AvoidConstParamsInDecls.cpp
24 ↗(On Diff #51596)

I suspect this will fail to build in MSVC 2013, since it doesn't support uniform initialization. But it might be just initializer lists that it doesn't support. We can try.

32 ↗(On Diff #51596)

"Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

This revision is now accepted and ready to land.Mar 29 2016, 9:42 AM
fowles updated this revision to Diff 51954.Mar 29 2016, 10:53 AM
fowles marked 2 inline comments as done.
fowles edited edge metadata.
  • review comments
  • rename variables and remove MSVC compat issues
This revision was automatically updated to reflect the committed changes.

Actually, we missed one thing: may I ask you to update docs/ReleaseNotes.rst with a short description of the new check?

fowles added a subscriber: fowles.Mar 30 2016, 7:01 AM

My attempts to do this end with:

$ arc diff
Linting...
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
SKIP STAGING Unable to determine repository for this change.
Exception
ERR_CLOSED: This revision has already been closed.
(Run with --trace for a full exception trace.)

You can submit the release notes changes in a new patch.

that is what I was trying to do. I can't seem to make arc play nice.

Maybe you need to rebase first? I haven't used arc.