This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a new clang-tidy check for trivially copyable types passed by const reference
Needs ReviewPublic

Authored by jmarrec on Aug 11 2021, 6:22 AM.

Diff Detail

Event Timeline

jmarrec created this revision.Aug 11 2021, 6:22 AM
jmarrec requested review of this revision.Aug 11 2021, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 6:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jmarrec updated this revision to Diff 365735.Aug 11 2021, 6:31 AM

Added content to the clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst file

Quuxplusone added inline comments.
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
22

D107873 is related.

I'd like to see some tests/discussion around large types, e.g.

struct Widget { int a[1000]; };
void f(const Widget&);

(I think D107873 at least makes a conscious attempt to get the "would it be passed in registers?" check right.)

I'd like to see some tests/discussion around generic code, e.g.

template<class T>
struct Max {
    static T max(const T& a, const T& b);
};
int x = Max<int>::max(1, 2);  // passes `int` by const reference, but this is fine
jmarrec added inline comments.Aug 11 2021, 7:54 AM
clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp
4–6

This is now failing after I applied clang-format. int f1(const int& i) would correctly produce int f1(int i);, now it does int f1(inti);... I need to find a way to fix that.

jmarrec added inline comments.Aug 11 2021, 8:10 AM
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
22

Should I close this one in favor of https://reviews.llvm.org/D107873?

jmarrec updated this revision to Diff 365778.Aug 11 2021, 9:36 AM

Added a MaxSize option. defaults to 16. Also skip templates. like in D107873

Dealt with the f(int &i) case instead of f(int& i)

jmarrec updated this revision to Diff 365779.Aug 11 2021, 9:38 AM

Forgot to run clang format on the changes

cjdb added a subscriber: cjdb.Aug 11 2021, 1:47 PM
cjdb added inline comments.
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
22

Up to you. There's always a chance that D107873 doesn't get approved. What I find most interesting is that we were independently working on this at the same time :-)

jmarrec added inline comments.Aug 13 2021, 1:09 AM
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
22

@cjdb I guess it's uncanny that we would both decide to do it at the same time. A bit unfortunate too probably, I could have saved a few hours :) but I learned something about clang-tidy so I don't mind at all!

Yours (D107873) is definitely better documented, you handle to opposite case (I only deal with const-ref -> value, not value -> const ret) and I just borrowed your implementation for MaxSize / ignoring shared_ptr

As far as I can tell from a cursory look, there are a couple of things I'm doing that you aren't:

  • I am actually using Options, I don't think you are yet, even for MaxSize. I see from your documentation file that you plan to add a few
  • The RestrictToBuiltInTypes I find an interesting option too
  • I am removing the const as well in addition to the &. I guess you are 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...).

Perhaps we could discuss adding the above to your PR and consolidate over there to make a nice, feature-complete check?

cjdb added inline comments.Aug 13 2021, 8:26 AM
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
22

Sounds good.

JonasToth retitled this revision from Add a new clang-tidy check for trivially copyable types passed by const reference to [clang-tidy] Add a new clang-tidy check for trivially copyable types passed by const reference.Aug 15 2021, 3:10 AM
whisperity added inline comments.
clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h
18–25
clang-tools-extra/docs/ReleaseNotes.rst
78
79
clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst
7
9
jmarrec updated this revision to Diff 368881.Aug 26 2021, 7:49 AM

documentation updates