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

[Whisperity via Phabricator via cfe-commits]

whisperity marked 6 inline comments as done.
whisperity added inline comments.
Herald added a subscriber: nullptr.cpp.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+                                       const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");
----------------
aaron.ballman wrote:
> Some interesting test cases to consider: varargs functions and K&R C functions
> K&R C functions

Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported! **But:** thanks to Clang supporting it properly in the AST, the checker works out of the box!

Given

```
int foo(a, b)
  int a;
  int b;
{
  return a + b;
}
```

We get the following output:

```
/tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
  int a;
  ^
/tmp/knr.c:2:7: note: the first parameter in the range is 'a'
  int a;
      ^
/tmp/knr.c:3:7: note: the last parameter in the range is 'b'
  int b;
      ^
```

(even the locations are consistent!)

Should I add a test case for this? We could use a specifically C test case either way eventually...

-----

> varargs functions

This is a bit of terminology, but something tells me you meant the //variadic function// here, right? As opposed to type parameter packs.

Given

```
int sum(int ints...) {
  return 0;
}
```

the AST looks something like this:

```
`-FunctionDecl 0x56372e29e258 <variadic.cpp:1:1, line:3:1> line:1:5 sum 'int (int, ...)'
  |-ParmVarDecl 0x56372e29e188 <col:9, col:13> col:13 ints 'int'
```

Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the `va_` standard functions' semantics either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560