This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with optionally considering differently qualified types mixable
ClosedPublic

Authored by whisperity on Feb 9 2021, 10:24 AM.

Details

Summary

Adds a relaxation option QualifiersMix which will make the check report for cases where parameters refer to the same type if they only differ in qualifiers.

This makes cases, such as the following, not warned about by default, produce a warning.

void* memcpy(void* dst, const void* src, unsigned size) {}

The reasoning behind this is that several projects, code styles, and preferences might consider T and const T fundamentally different types. The related C++ Core Guidelines rule itself shows an example where void T*, const void T* is considered a "good solution".

However, unless people meticulously const their local variables, unfortunately, even such a function carry a potential swap:

T* obj = new T; // Not const!!!
void* buf = malloc(sizeof(T));

memcpy(obj, buf, sizeof(T));
//     ^~~  ^~~ accidental swap here, even though the interface "specified" a const.

Of course, keeping your things const where appropriate results in an error:

const T* obj = new T;
void* buf = malloc(sizeof(T));

memcpy(obj, buf, sizeof(T));
// error: 1st argument ('const T*') loses qualifiers

Due to the rationale behind this depending on project-specific guidelines and preferences, the modelling is introduced as a check option. The default value is false, i.e. T* and const T* are considered unmixable, distinct types.

Originally, the implementation of this feature was part of the very first patch related to this check, D69560 (diff 259320). It has been factored out for clarity and to allow more on-point discussion.

Diff Detail

Event Timeline

whisperity created this revision.Feb 9 2021, 10:24 AM
whisperity requested review of this revision.Feb 9 2021, 10:24 AM
  • [NFC] Reformat the documentation so option values are in single backticks.
aaron.ballman added inline comments.Mar 16 2021, 9:50 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
181

Hmm, use of << is a bit novel but not entirely indefensible. I guess my initial inclination is that you're combing this information into the mix data and so an overload of | was what I was sort of expecting to see here (despite it not really being a bitmask operation). These aren't strong opinions, but I'm curious if you have thoughts.

291
337

isAnyPointerType() for ObjC pointers? Should we care about rvalue references here similar to pointers?

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
39
40
whisperity added inline comments.Mar 16 2021, 10:36 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
181

I looked at all the possible operator tokens and this seemed the most appropriate. It points to the left, where qualifiers are in LLVM's programming style ("west const" and all that). Because sometimes it's variables that get passed to these functions, seeing from the get-go that it's not meddling with the flags but rather with the types involved seemed appropriate to emphasise.

337

The reasoning to not be special about && goes back to D95736. If you are given any combination of T, T&, T&& and const T& parameters and some values of (essentially) T, it's only T and const T& that always mix for every potential value of T.

T&& won't bind variables, and T& won't bind temporaries. So their potential to mix is an inherent property of the call site where the mix might happen. If one of them is T and the other is T& and you happen to pass one variable and one literal, and you happen to swap these two, you'll get a compile error.

If both parameters are T&&, LType == RType for a trivial mix catches it early on.

aaron.ballman added inline comments.Mar 16 2021, 10:59 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
181

Okay, that seems reasonable enough to me, thanks!

337

Ah, thank you for the explanation about rvalue references, that's helpful!

As for ObjC... after thinking about it a bit more, I'm on the fence. On the one hand, ObjC is C + extra features, and you can swap parameters in C. On the other hand, ObjC tends to use message sending an awful lot where names the parameters are used at the call site, so this check is less interesting in an ObjC code base in some ways. I think it might make more sense to ignore ObjC for now and try to tackle that in the future (perhaps at user request).

whisperity marked 6 inline comments as done.
This revision is now accepted and ready to land.Mar 17 2021, 6:09 AM
whisperity set the repository for this revision to rG LLVM Github Monorepo.

[NFC] Rebase.

NFC: Rebase & update.

NC! Recreate diff to try and get the buildbots running...

NC Fix lint mishap that occurred during previous rebase.