[PATCH] D41077: [analyser] different.CallArgsOrder checker implementation

[Peter Szecsi via Phabricator via cfe-commits]

szepet added subscribers: alexfh, szepet.
szepet requested changes to this revision.
szepet edited reviewers, added: xazax.hun, szepet; removed: dergachev.a.
szepet added a comment.
Herald added a subscriber: rnkovacs.

Hi Alexey!

Thank you for working on this!

Some general comments on the patch:

1. Please upload the changes with the context included (git flag -U99999) which could help the review process.
2. Use the LLVM Coding Guidelines on the tests as well (Start variable names with a capital letter, etc )
3. Ping here, not necessary on the cfe-dev mailing list ;)
4. FYI: There is a similar check under review which uses only the AST provided information and implemented as a tidy-checker: https://reviews.llvm.org/D20689 (As I see your checker does not uses symbolic execution provided features. So, it would probably worth thinking about if the analyzer is the project where the checker should be implemented. However, @dcoughlin and @alexfh have more insight on the answer to this question. What do you think? )
5. In overall, please add comments to the function which describes its purpose. Mainly on heuristic functions, it can help to understand it more easily. Also, could you provide some results of the current heuristics, what are the false positive rates on real projects like LLVM, FFmpeg, etc? I am quite interested in that.

Some comments added inline but they are basically the above mentioned things.



================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:57
+
+    bool isNamedLike(StringRef Name) const {
+      if (SimplifiedName.size() < Name.size()) {
----------------
It looks like that [[ https://en.wikipedia.org/wiki/Levenshtein_distance | Levenshtein ]] edit distance could come handy in cases like this. It is already implemented as a method `edit_distance` on StringRef.


================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:164
+
+StringRefVec ParamsGroup::rtrimSame(StringRefVec StrVec) {
+  if (StrVec.empty())
----------------
Please add a comment to this function which describes its input, output, purpose.


================
Comment at: test/Analysis/call-args-order.c:4
+struct Viewport {
+  int width;
+  int height;
----------------
Please use capital starting letter on variables. On the other test file as well.


Repository:
  rC Clang

https://reviews.llvm.org/D41077