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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
