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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits