[PATCH] D106361: [clang-tidy] Fix crash and handle AttributedTypes in 'bugprone-easily-swappable-parameters'

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D106361#2890458 <https://reviews.llvm.org/D106361#2890458>, @aaron.ballman wrote:

> The rename from `CommonType` to `CoreType` could probably be done as a separate NFC to make the review a bit easier, if we decide that's a good terminology switch. I'm not certain switching from `common` to `core` makes things more clear though; what's the distinction trying to be made?

Not much, just tried to dance around the fact that the diagnostic emitted for this is, put simply, crappy. I'm coming up with a better solution, and indeed, let's fix the //issue// at hand now and focus on the //improvements// later.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:476-477
+
+    QualType NewCoreType = CoreType;
+    NewCoreType.addFastQualifiers(Quals.getFastQualifiers());
+    NewCoreType.getQualifiers().addQualifiers(Quals);
----------------
aaron.ballman wrote:
> 
Actually, the suggestion is also bad. `getQualifiers()` returns a **copy** on which adding is a moot operation... Turns out you can use `ASTContext` to //create// a specifically qualified type for you.

Now if there was a way to express this in the type of `getQualifiers()`, to warn you //"Don't make the mistake of thinking this would CHANGE anything!"//...


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:2201
             DiagText = "after resolving type aliases, '%0' and '%1' share a "
-                       "common type";
+                       "core type";
           else
----------------
aaron.ballman wrote:
> I don't know that "core type" is a term of art that will convey much meaning to users in a diagnostic message. Do we need to change the wording of the diagnostic?
The wording needs to be changed, but not in this particular way. I started working on a follow-up refactoring patch in general and some changes made it into this diff. I'll revert these from this patch and will work everything related to that into a new one.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:362-364
+// FIXME: The last diagnostic line is a bit bad: the core type should be a
+// pointer type -- it is not clear right now, how it would be possible to
+// properly wire a logic in that fixes it.
----------------
aaron.ballman wrote:
> Yeah, the diagnostic here is a bit unfortunate. This was an existing deficiency with the check though, right? I assume it also impacts references and not just pointers?
I will look into this (and improving the diagnostics in general) in a follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106361