Page MenuHomePhabricator

[clang-tidy] Add misc-mutating-copy check
ClosedPublic

Authored by gbencze on Nov 10 2019, 8:00 AM.

Diff Detail

Event Timeline

gbencze created this revision.Nov 10 2019, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2019, 8:00 AM

Have you run your check over the LLVM/clang source base and seen true positives/false positives?

clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
6 ↗(On Diff #228606)

to and to

Have you run your check over the LLVM/clang source base and seen true positives/false positives?

I tested it on the Xerces and Bitcoin codebases and did not get any warnings.

Did you consider to warn on copy constructors/assignment operators take a their arguments by non-const reference, and suggest the user to turn them into const references? This seems like a more useful (and easier) check to me.
The link above contains Ideally, the copy operator should have an idiomatic signature. For copy constructors, that is T(const T&); and for copy assignment operators, that is T& operator=(const T&);..

The only remaining case are then mutable members which are quite rare.

Did you consider to warn on copy constructors/assignment operators take a their arguments by non-const reference, and suggest the user to turn them into const references? This seems like a more useful (and easier) check to me.
The link above contains Ideally, the copy operator should have an idiomatic signature. For copy constructors, that is T(const T&); and for copy assignment operators, that is T& operator=(const T&);..

The only remaining case are then mutable members which are quite rare.

I haven't really considered it as the non-compliant example has the idiomatic signature, but it probably would be a good addition to the check. misc-unconventional-assign-operator already seems to do this for the assignment operator.

If this is CERT rule, why check is not placed in relevant module?

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

Please synchronize with first statement in documentation.

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

gbencze updated this revision to Diff 228758.Nov 11 2019, 12:37 PM

Update documentation and release notes.

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

Misc is just heap of unsorted checks, so CERT is definitely better place.

If this is CERT rule, why check is not placed in relevant module?

To be honest I was hoping for some feedback on this as I wasn't sure what the best place for this check would be. Quite a few CERT rules seem to be implemented in other modules and have cert aliases.
Do you think this check should be moved there or should I just add an alias?

I'd probably implement this only in the cert module, but if we wanted to expose it outside of there as well, I'd suggest bugprone.

clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
107 ↗(On Diff #228758)

I think a case like this should also diagnose:

class S {
  int a;
public:
  void fine_func() const;
  void questionable_func();
  void bad_func() { a = 12; }

  S(S& other) : a(other.a) {
    other.fine_func(); // This is fine
    other.bad_func(); // This is a problem
    other.questionable_func(); // Not certain what to do here.
  }

  bool operator==(const S& other) { return a == other.a; }
};
mgehre removed a subscriber: mgehre.Nov 15 2019, 12:34 PM
gbencze updated this revision to Diff 229720.Nov 17 2019, 8:02 AM

Moved check to CERT module.
Added warnings for calling mutating member functions.

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

aaron.ballman added inline comments.Nov 18 2019, 2:00 PM
clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
74–80

You can elide the curly braces here per our usual coding conventions.

clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
12

Please add the newline to the end of the file.

clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
2

This -std looks incorrect to me -- you can remove it entirely (we already default to later than C++11 anyway).

150

Please add the newline to the end of the file.

gbencze updated this revision to Diff 229925.Nov 18 2019, 2:55 PM

Formatting changes (curly braces, newline at EOF)
Remove incorrect flag from test

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

You're right, the current version does not cover mutations through pointers and references. I'm not sure how common these would be, but ExprMutAnalyzer seems like a great option if we wanted to add support for those cases as well.

gbencze marked 5 inline comments as done.Nov 25 2019, 4:38 AM

Mark comments as Done.

Charusso accepted this revision.Dec 8 2019, 11:33 PM

I think it is fine, but please let us wait with the final thoughts from @aaron.ballman.

clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
22

What about just "assignment" and "member-call"?

64

I think hasDescendant() is more appropriate compared to the slower forEachDescendant().

This revision is now accepted and ready to land.Dec 8 2019, 11:33 PM
aaron.ballman accepted this revision.Dec 9 2019, 10:23 AM

There is a ExprMutAnalyzer that is able to find mutation of expressions in general (even though it is kinda experimental still). Maybe that should be utilized somehow? I think the current implementation does not cover when the address is taken and mutation happens through pointers/references in free standing functions, does it?

On the other hand it makes the check more complicated, slower. Additionally the most cases are catched with this version, i guess.

You're right, the current version does not cover mutations through pointers and references. I'm not sure how common these would be, but ExprMutAnalyzer seems like a great option if we wanted to add support for those cases as well.

It also doesn't cover mutations through function calls. e.g.,

void some_func(S &); // Could mutate the passed object

struct S {
  int i, j;
  S(S &s) : i(s.i), j(s.j) { some_func(s); }
};

However, I think the clang-tidy check will suffice for catching a significant percentage of mutating copy bugs so it seems reasonable to keep this rather than insist on a static analyzer check.

This revision was automatically updated to reflect the committed changes.