This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add 'performance-const-parameter-value-or-ref' for checking const-qualified parameters
Needs ReviewPublic

Authored by cjdb on Aug 10 2021, 4:43 PM.

Details

Summary

Checks `const`-qualified parameters to determine if they should be
passed by value or by reference-to-`const` for function definitions.
Ignores proper function declarations, parameters without `const` in
their type specifier, and dependent types.

Diff Detail

Event Timeline

cjdb created this revision.Aug 10 2021, 4:43 PM
cjdb requested review of this revision.Aug 10 2021, 4:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 10 2021, 4:43 PM
cjdb added a comment.Aug 10 2021, 4:44 PM

Work left to do in this patch: add user options. Figured getting a non-config version up for review first would be a good idea.

cjdb added a comment.Aug 10 2021, 5:25 PM

Those CI failures are kinda embarrassing. Is there any way to leverage clang-format in a case like this?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp
58

Please use static, not anonymous namespace for functions.

59

Please don't use auto unless type is spelled in same statement or iterator.

clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h
2

See other headers as example for C++ definition for code editors.

19

Please separate with newline.

44

Seems to be unused.

47

Please separate with newline.

clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
17

Please remove newline.

68

Please use single back-tick for option values.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 9:56 PM
Eugene.Zelenko removed a project: Restricted Project.Aug 10 2021, 9:56 PM
Eugene.Zelenko removed a subscriber: llvm-commits.
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 9:56 PM
jmarrec added inline comments.
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
91
102
105

typo here, not sure what the intent was.

cjdb updated this revision to Diff 366456.Aug 14 2021, 2:31 PM
cjdb marked 9 inline comments as done.
  • adds user options
  • removes use of auto
  • fixes header comment
  • removes unused type
  • fixes documentation
cjdb marked an inline comment as done.Aug 14 2021, 2:33 PM

@jmarrec I believe you wanted to import a discussion from D107900?

clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp
58

Hmm... there are lots of instances of namespace { in clang-tidy.

clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
105

I think this is something I put at the end first, and then moved to the start of the sentence, but forgot to delete.

cjdb updated this revision to Diff 366457.Aug 14 2021, 2:34 PM
cjdb retitled this revision from [WIP][clang-tidy] adds a const-qualified parameter check to [clang-tidy] adds a const-qualified parameter check.

removes WIP tag from patch name

whisperity added inline comments.
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.cpp
58

Yeah, because you can't do static class/struct X.

73

QualType is small (AFAIK it's just 2 pointers), no need for a reference on this, it can live on the stack.

92

This reminds me eerily of performance-unnecessary-value-param!

clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h
19–22
clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
59

(Minor suggestion: maybe MaxSmallSize would be a better name here than SmallMaxSize.)

102–103

Will GitHub and Godbolt outlive LLVM? I am not comfortable with depending on external sources (such as shortlinks like that git.io one that I was a bit anxious to click...) to explain decisions. I get it that the code is long to be put into the documentation, though... But perhaps the benchmark's tables could go in here, at the very bottom, as an appendix.

whisperity retitled this revision from [clang-tidy] adds a const-qualified parameter check to [clang-tidy] Add 'performance-const-parameter-value-or-ref' for checking const-qualified parameters.Aug 16 2021, 12:23 AM
whisperity removed projects: Restricted Project, Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 12:23 AM

It seems like there may be some considerable overlap between this check and the one proposed in https://reviews.llvm.org/D54943. I know the other check is more about C++ Core Guidelines so it's not perfect overlap. But I do wonder if it'd make sense to combine the efforts given that the goal of both checks is to find const correctness issues. Do you think there's a chance for sharing implementation efforts here?

MTC added a subscriber: MTC.Aug 16 2021, 6:34 PM

Imported some discussion from https://reviews.llvm.org/D107900#inline-1029358. Sorry it took that long

clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h
40

https://reviews.llvm.org/D107900#inline-1029358

On mine, I had another option RestrictToBuiltInTypes which I find an interesting option. It has a builtinType() matcher in the registerMatchers function

43

Perhaps two additional options to turn off either "side" would be helpful? eg CheckPassByValueIsAppropriate and CheckPassByRefIsAppropriate, both defaulted to true

clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
44

https://reviews.llvm.org/D107900#inline-1029358

I am removing the const as well in addition to the &. I guess you may be just relying on using it in conjunction with readability-avoid-const-params-in-decls but I think that makes it harder to use (you can't just do -checks=-*,<yourcheck> you have to remember to add the readability-avoid-const-params-in-decls too...).

Taht being said, perhaps that's how it's meant to be...

cjdb added inline comments.Sep 1 2021, 2:58 PM
clang-tools-extra/clang-tidy/performance/ConstParameterValueOrRef.h
40

Could you please elaborate on why you find this interesting? I don't see any benefit to ignoring struct S { char c; }.

clang-tools-extra/docs/clang-tidy/checks/performance-const-parameter-value-or-ref.rst
44

I don't know if our goals are aligned here. This patch intends to preserve constness in all cases. If a user defines a function as

void f(int const& x) { }

Then there's a good chance that they want the constness. Removing that constness changes the meaning of the program since x would then be modifiable. What's deferred to readability-avoid-const-params-in-decls is const-qualified value parameters in forward declarations, which isn't really in scope here, since forward declarations are used to overrule this check's analysis.