[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 12 inline comments as done.
whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:657-660
+    } else {
+      ParamTypes.push_back(QualType());
+      ParamNames.push_back(StringRef());
+    }
----------------
whisperity wrote:
> aaron.ballman wrote:
> > Can this case happen?
> Oops... It seems I posted the updated patch right where you were writing more comments and we got into a data race. Which case are you referring to? It's now affixed to a `diag(` call for me...
Oh, nevermind, there is a button that shows me the older diff where it's aligned properly.

And yeah, it seems it can't, `getParamDecl` always returns a `ParmVarDecl`. Weird issues might arise when the vectors that are built here get out of sync (such as the issue we had with `operator()` calls before I fixed it!), so I understood the reason behind keeping the two functions parallel with each other in terms of pure visuals, even.


================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:743-744
+
+    if (AreNamesSimilar)
+      break;
+
----------------
aaron.ballman wrote:
> Any reason not to move this below the `switch` and use `=` instead of `|=` within the cases? (Or return from the cases directly?)
Returning from the cases directly is a bad idea because we want to try all heuristics and only say `false` if none of them matches.
But this break in a very bad location, I agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689