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