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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58
+
+namespace {
+
----------------
aaron.ballman wrote:
> Is there a need for the anonymous namespace? (We typically only use them when defining a class and try to make them as narrow in scope as possible, and use `static` for function declarations instead.)
There will be two major parts, the modelling (and which is extended down the line with new models like implicit conversions) and the filtering (which throws away or breaks up results). Should I cut out the anonymous namespace? How about the inner namespaces, may they stay?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+  MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!
----------------
aaron.ballman wrote:
> Does using the fixed underlying type buy us something? As best I can tell, this will introduce a fair amount of implicit promotions to `int` anyway.
Will it? I thought if I use the proper LLVM bitmask/flag enum thing it will work properly. For now, it is good for the bit flag debug print, because it only prints 8 numbers, not 32.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+  void sanitise() {
+    assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+    // TODO: There will be statements here in further extensions of the check.
----------------
aaron.ballman wrote:
> Heh, I don't know if we have any guidance on US English vs British English spellings. ;-)
Reflexes, my bad. (Similarly how I put the pointer `*` to the left side, where Stroustrup intended. At least my local pre-commit hooks take care of that.) Will try to remember this.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+      Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};
----------------
aaron.ballman wrote:
> Hmm, one downside to using the printing policy to get the node name is that there can be alternative spellings for the same type that the user might reasonably expect to be applied. e.g., if the user expects to ignore `bool` datatypes and the printing policy wants to spell the type as `_Bool`, this won't behave as expected.
But then the diagnostic is inconsistent with what the compiler is configured to output as diagnostics, isn't it? Can I stringify through Clang with some "default" printing policy?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+    // Unfortunately, undersquiggly highlights are for some reason chewed up
+    // and not respected by diagnostics from Clang-Tidy. :(
+    diag(First->getLocation(), "the first parameter in the range is '%0'",
----------------
aaron.ballman wrote:
> Can you post a screenshot of what you mean?
Given

```
Diag << SourceRange{First->getSourceRange().getBegin(), Last->getSourceRange().getEnd()};
```

The diagnostic is still produced with only a `^` at the original `diag(Location)`, ignoring the fed `SourceRange`:

```
/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
                 ^
```

Instead of putting all the squigglies in as the range-location highlight, like how `Sema` can diagnose:

```
/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
                 ^~~~~~~~~~~~~~~~~~~
```

I would not want to put additional note tags in an otherwise already verbose output.

Back in 2019 I was investigating this issue specifically for another checker I was working on, but hit the wall... Somewhere deep inside where Tidy diagnostic stuff is translated and consumed to Clang stuff, it goes away. It shouldn't, because there are statements that seemingly copy the `Diag.Ranges` array over, but it still does not come out.

... EDIT: I found [[ http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the relevant mailing list post ]].


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+  /// The minimum length of an adjacent swappable parameter range required for
----------------
aaron.ballman wrote:
> Do these data members need to be public?
Yes. The modelling heuristics will have a bunch of recursive functions which all need the config variables (okay, not the `MinimumLength` one, but the other 4 or 5 that will be introduced) and giving them every time by copy would introduce its own adjacent swappable `bool, bool` parameter sequence issue.

The previous full version of the check which I'm incrementally reworking has signatures like:

```
static MixupData HowPossibleToMixUpAtCallSite(const QualType LType,
                                              const QualType RType,
                                              const ASTContext &Ctx,
                                              const bool CVRMixPossible,
                                              const bool ImplicitConversion,
                                              const bool IsUserDefinedConvertersOtherwisePossible);

static MixupData RefBindsToSameType(const LValueReferenceType *LRef,
                                    const Type *T, const ASTContext &Ctx,
                                    const bool IsRefRightType,
                                    const bool CVRMixPossible,
                                    const bool ImplicitConversionEnabled);
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:14
+
+S *allocate() { return nullptr; }                           // NO-WARN: 1 parameter.
+void allocate(S **Out) {}                                   // NO-WARN: 1 parameter.
----------------
aaron.ballman wrote:
> 
Good catch! This is what you get when you copy-paste. 😁 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560