[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:207
+
+      LLVM_DEBUG(QT.dump());
+
----------------
Actually this is one of those debug prints that should be removed and remained in here by accident.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs<FunctionProtoType>() || RType->getAs<FunctionProtoType>()) {
+    // Unfortunately, the canonical type of a function pointer becomes the
+    // same even if exactly one is "noexcept" and the other isn't, making us
+    // give a false positive report irrespective of implicit conversions.
+    LLVM_DEBUG(llvm::dbgs()
----------------
@aaron.ballman Getting ahead of the curve here. I understand this is ugly. Unfortunately, no matter how much I read the standard, I don't get anything of "canonical type" mentioned, it seems to me this concept is something inherent to the model of Clang.

Basically why this is here: imagine a `void (*)() noexcept` and a `void (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a //one-way// implicit conversion from the `noexcept` to the non-noexcept ("noexceptness can be discarded", similarly to how "constness can be gained")
Unfortunately, because this is a one-way implicit conversion, it won't return on the code path earlier for implicit conversions.

Now because of this, the recursive descent in our code will reach the point when the two innermost `FunctionProtoType`s are in our hands. As a fallback case, we simply ask Clang "Hey, do //you// think these two are the same?". And for some weird reason, Clang will say "Yes."... While one of them is a `noexcept` function and the other one isn't.

I do not know the innards of the AST well enough to even have a glimpse of whether or not this is a bug. So that's the reason why I went ahead and implemented this side-stepping logic. Basically, as the comment says, it'lll **force** the information of "No matter what you do, do NOT consider these mixable!" back up the recursion chain, and handle it appropriately later.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp:250-260
+using NonThrowingFunction = void (*)() noexcept;
+
+struct NoexceptMaker {
+  NoexceptMaker(void (*ThrowingFunction)());
+  // Need to use a typedef here because
+  // "conversion function cannot convert to a function type".
+  // operator (void (*)() noexcept) () const;
----------------
Now here the warning is justified, however. All these disinfectant-stinking examples are to ensure that. In this case, in our hands, on the first level, we got the function pointer, and the class. And it **is** the implicit conversion modeller that will eventually ask "Can I give that noexcept function to that constructor taking a non-noexcept?" and in that case, on the level of function pointers, the answer is //yes//, so there will be a warning made.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:336-337
+
+void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void (*Throwing)()) {}
+// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise.
----------------
Without this side-stepping logic mentioned above, even **without this patch** (so back to the "Ye Olde Strict Type Equality" mode), this particular function would be warned about, which is a definite false positive.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75041/new/

https://reviews.llvm.org/D75041