alexfh added a comment.
Thanks for the great work! This is mostly looking fine, but I've added a few
comments. I could only make a quite superficial review so far, but I'll try to
take a deeper look into this soon.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:70
+ static constexpr std::size_t WidthWithPadding = Width + (Width / 4);
+ char S[WidthWithPadding];
+ for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) {
----------------
I think, this function is not needed (see the comment below), but if it is, a
single std::string and in-place std::reverse() would be better, imo.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:84-87
+/// Formats the bit representation of a numeric type as a string in groups of
4.
+template <typename T> static inline std::string formatBits(T &&V) {
+ return formatBitsImpl<sizeof(T) * CHAR_BIT>(V);
+}
----------------
I suppose that textual representation of the flags would be easier to
understand in debug logs than just bits. Maybe single characters or other
abbreviations, if this needs to be compact.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+ FB(None, 1), //< Mix between the two parameters is not possible.
+ FB(Trivial, 2), //< The two mix trivially, and are the exact same type.
+ FB(Canonical, 3), //< The two mix because the types refer to the same
+ // CanonicalType, but we do not elaborate as to how.
+
----------------
I'd switch to scoped enums (`enum class`), remove the macro, and use simpler
code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+ void sanitize() {
+ assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");
----------------
What should this method do and how it should be used? Same for Mix::sanitize()
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:295-297
+ if (llvm::any_of(
+ Check.IgnoredParameterNames,
+ [NodeName](const std::string &E) { return NodeName == E; })) {
----------------
Would `llvm::find(Check.IgnoredParameterNames, NodeName) !=
Check.IgnoredParameterNames.end()` work?
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+
+ StringRef NodeTypeName = [Node]() {
+ const ASTContext &Ctx = Node->getASTContext();
----------------
No need for parentheses here.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:313-320
+ if (B.isMacroID()) {
+ LLVM_DEBUG(llvm::dbgs() << "\t\tBeginning is macro.\n");
+ B = SM.getTopMacroCallerLoc(B);
+ }
+ if (E.isMacroID()) {
+ LLVM_DEBUG(llvm::dbgs() << "\t\tEnding is macro.\n");
+ E = Lexer::getLocForEndOfToken(SM.getTopMacroCallerLoc(E), 0, SM, LO);
----------------
Is there a chance that you're trying to reimplement a part of
Lexer::makeFileCharRange functionality here?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+
----------------
Does the top-level const change anything with regard to the "mixability"? It's
a purely implementation-side difference, callers could still swap parameters.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69560/new/
https://reviews.llvm.org/D69560
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
