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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this looks good to me. @alexfh, you had raised questions about this meeting the quality bar. I think those concerns are valid in the abstract (swapped argument checking requires heuristics), but I'd argue that most existing code bases that actually have swapped args finds those bugs in production, so the rate of true positives on existing code is often small. I see the value from this check coming from checking new code as it's written, as part of a CI pipeline for instance. With that in mind, do you still have concerns about this check?



================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:801
+  }
+  llvm_unreachable("Unhandled heuristic kind");
+}
----------------
whisperity wrote:
> aaron.ballman wrote:
> > This looks pretty reachable to me in the case where there's no bound.
> I'm not sure if that is the case. I added the `llvm_unreachable` so we don't get a warning about the function not having a return value on every code path. The `switch` covers **all** potential heuristics that are in the check //right now//, but if we add a new heuristic (to the enum) and forget to write it in, we will get a `-Wswitch` warning here. A `default` case doesn't apply here, further developers should be encouraged to wire new heuristics in properly.
Oops, ignore my think-o! I somehow read the switch as switching over `Threshold` and not `H` and confused myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689