[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2747092 <https://reviews.llvm.org/D69560#2747092>, @alexfh wrote:

> BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)
>
> It would be really nice to get pre-merge checks to work for the patch.

Put simply, I think I haven't rebased the patch since a few changes ago. The past few changes were NFC/lint stuff only.



================
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.
+
----------------
alexfh wrote:
> I'd switch to scoped enums (`enum class`), remove the macro, and use simpler code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
`enum class` is a good idea!

Unfortunately, I'm not really sold on the hexa literals... It makes the code confusing the read, because you have to keep in mind which range the hexa literals mean, and when there is a shift in position, e.g. if you read `0x20` you have to count out that okay, that's 2nd position, so at least 16, so at least the 4th bit, but it is two, so you shift it again to the right, so in the bit pattern it turns on the 5th bit actually.
It makes it very easy to misread or misinterpret things, especially when trying to work on the source code.
(In the last extension patch, the number of elements in the bit mask go up to the 8th or the 9th bit, I don't remember exactly at the top of my head.)

Compared to this, the current text when read immediately tells you which "decimal" position you should expect the appropriate bit to be toggled on, if the flag is there.

Binary literals, on the other hand, would be amazing to use here, sadly we're only in C++14.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+    assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");
----------------
alexfh wrote:
> What should this method do and how it should be used? Same for Mix::sanitize()
Once there are more flags in the bitmask (not just "None" and "Trivial"), this method deals with sanitising the flags that are toggled on and off by the recursive descent on the type tree.

For example, if there is a typedef involved (see D95736), it will toggle on the typedef flag, and start investigating the aliased type. This subsequent investigation might figure out that the aliased type is trivial, or it is distinct. In case it is distinct, that level of the recursion will turn on the "Not mixable" flag. In this case, when the investigation returns completely, we will have "Typedef" and "None" both turned on. This method will throw away everything but the "None" bit, so when the data is returned to the diagnostic-generating part of the check, it will know what to do.


================
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);
----------------
alexfh wrote:
> Is there a chance that you're trying to reimplement a part of Lexer::makeFileCharRange functionality here?
Does that expand the token length properly? I took this idiom of `getLocForEndOfToken` from various other Tidy checks... basically, anything that deals with replacing some sort of textual range in the code calls to this function. I need to have the exact ending location so I can obtain the full text, as appears in the source file. In this case, to compare against the user's input.

E.g. here is one example from `utils/UsingInserter.cpp`:

```
  32   │ Optional<FixItHint> UsingInserter::createUsingDeclaration(
  33   │     ASTContext &Context, const Stmt &Statement, StringRef QualifiedName) {
/* ... */
  42   │   SourceLocation InsertLoc = Lexer::getLocForEndOfToken(
  43   │       Function->getBody()->getBeginLoc(), 0, SourceMgr, Context.getLangOpts());
```

A quick grep for `makeCharRange` in the repository didn't turn up any significant result, apart from one unit test.


================
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.
+
----------------
alexfh wrote:
> Does the top-level const change anything with regard to the "mixability"? It's a purely implementation-side difference, callers could still swap parameters.
Unfortunately, coding styles differ whether people mark local variables const or not, across projects. Even with pointers: in some cases, a swapped call to `memcpy` might be caught if the user made one of the pointers passed already `const void*` instead of just `void*`.

D95736 deals with implementing the logic for this, it is exposed as a user-configurable option whether they want to consider const stuff the same as non-const stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560