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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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");
----------------
whisperity wrote:
> 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.
> 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!

Ah, to be innocent again, how I miss those days. :-D

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

I think it'd be a useful case, but the one I was specifically more concerned with is:
```
// N.B.: this is C-specific and does not apply to C++.
void f();

int main(void) {
  f(1, 2, 3.4, "this is why we can't have nice things");
}
```
where the function has no prototype and so is treated as a varargs call.

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

Yes, sorry for being unclear, I am talking about variadic functions.

> 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.

Well, that's what I'm wondering, really. The arguments are certainly easy to swap because the type system can't help the user to identify swaps without further information (like format specifier strings). However, the checking code would be... highly unpleasant, I suspect. My intuition is to say that we don't support functions without prototypes at all (we just silently ignore them) and that we only check the typed parameters in a variadic function declaration (e.g., we'll diagnose `void foo(int i, int j, ...);` because of the sequential `int` parameters, but we won't diagnose `void foo(int i, ...);` even if call sites look like `foo(1, 2);`). WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560