This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check
ClosedPublic

Authored by carlosgalvezp on Jun 2 2022, 7:27 AM.

Details

Summary

Flags uses of const-qualified and reference data members in structs.
Implements rule C.12 of C++ Core Guidelines.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Jun 2 2022, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 7:27 AM
carlosgalvezp requested review of this revision.Jun 2 2022, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 7:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Move logic from check to registerMatchers

dfrib added a subscriber: dfrib.Jun 3 2022, 1:43 AM
dfrib added inline comments.Jun 3 2022, 1:58 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp
26

Differentiate between lvalue reference and rvalue reference members using these terms instead of "ref" and "refref". E.g.:

  • RefMember -> LvalueRefMember
  • RefRefMember -> RvalueRefMember

(apply to all cases below).

41

ConstAndRefMembers

101

Should we test for array types also? E.g.:

template<int N> using Array = int[N];

struct ConstArrayMember {
  const Array<1> c;
};

struct LvalueRefArrayMember {
  Array<2>& lvr;   
};

struct ConstRefArrayMember {
  Array<3> const& clvr;
};

struct LvalueRefArrayMember {
  Array<4>&& rvr;   
};
122

Consider expanding with the the non-const lvalue ref case, e.g.

TemplatedOk<int> t4{};
TemplateRef<int&> t5{t4.t};
carlosgalvezp marked 4 inline comments as done.

Fix review comments

This comment was removed by carlosgalvezp.
Eugene.Zelenko added inline comments.Jun 8 2022, 7:17 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
30

Please add isLanguageVersionSupported.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst
11

Usually such links are placed at the end.

carlosgalvezp marked 2 inline comments as done.

Address review comments.
Rebase on latest main.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h
30

Done. I generated this check via add_new_check.py - should we add this there as well to avoid similar issues in the future (separate commit)?

I see some checks implement this function and some others don't, so it's not totally clear to me when is needed.

Remove copy-paste comment

Kind ping to reviewers :)

LegalizeAdulthood requested changes to this revision.Jun 22 2022, 2:06 PM

Tests and docs have moved to subdirectories by module name.

Please rebase onto HEAD and:

  • move test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp to test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
  • move docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst to docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  • make the target test-clang-extra to validate your tests still pass
  • make the target docs-clang-tools-html to validate your documentation links are correct
This revision now requires changes to proceed.Jun 22 2022, 2:06 PM

Rebase and fix directory structure for doc and test.

Fix test name

@LegalizeAdulthood I've addressed your comments, thanks for the clear instructions! One thing I didn't manage to do is build the target docs-clang-tools-html, it says it doesn't exist. I've enabled LLVM_BUILD_DOCS=ON in the CMake call - do you know if I need to enable something else?

PS: also the test-clang-extra doesn't exist, I typically run check-clang-tools-extra.

clang-tools-extra/docs/ReleaseNotes.rst
126

Sort new checks by check name.

127

This link is wrong, please install Sphinx and build the target docs-clang-tools-html to validate links

carlosgalvezp marked 2 inline comments as done.

Address review comments.

@LegalizeAdulthood Thanks for the review! I've now rebased and addressed your comments. I've also verify the docs with the command you suggested, I was missing -DLLVM_ENABLE_SPHINX in the cmake command.

carlosgalvezp added a comment.EditedJul 10 2022, 7:01 AM

@LegalizeAdulthood I've addressed your comments, is there anything else that should be fixed before landing the patch?

Ping to reviewers

Rebase onto latest main.

@njames93 Would you mind reviewing? Thanks!

njames93 added inline comments.Jul 28 2022, 8:09 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
103–104 ↗(On Diff #448323)

I feel like this could potentially confuse users. Maybe we should walk the alias to show where the type was declared to be const/reference.
Or, not a fan of this choice, don't diagnose these unless the user opts into this behaviour.

136–157 ↗(On Diff #448323)

This whole block displeases me. Warning on the instantiation isn't a great idea and could confuse users.
Would again need some expansion notes to explain why the warning is being triggered, especially when if for 99% of the uses one of these structs has a T which isn't const or a reference.
Failing that just disabling the check in template instantiations would also fix the issue.

Great feedback, thanks! I had some ideas on how to go around the issues, looking forward to your thoughts.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
103–104 ↗(On Diff #448323)

Sounds reasonable! I see that Clang compiler does not print an "alias stack" in detail, for example here:

https://godbolt.org/z/8sqE4fM1v

For the sake of consistency and simplicity, would it make sense to print something similar here? Example:

"warning: member 'c' of type 'ConstType' (aka 'const int') is const qualified"
136–157 ↗(On Diff #448323)

I agree with the sentiment 100%. Unfortunately it's currently the only way to test this kind of code in clang-tidy AFAIK, see for example other existing checks:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp#L332

I fully agree on needing expansion notes, just like the Clang compiler does. A very similar question about it was recently posted:

https://discourse.llvm.org/t/clang-tidy-diagnostics-for-template-code/62909

In that sense I believe it would make sense to try and focus the efforts into implementing a good centralized solution instead of having it duplicated on different checks.

Until then, I believe it should be fairly easy to clarify the actual type as proposed in the above comment for aliases.

What do you think?

  • Display type information in the diagnostic.
  • Rebase onto latest main.

@njames93 I've updated the patch with extra type information, let me know if you think it's good enough!

bigdavedev accepted this revision.Aug 11 2022, 1:28 AM

LGTM. All comments have been addressed. Any improvements to the diagnostics can be made down the line depending on how users feel about them.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 11 2022, 1:33 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.