[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Varju Janos via Phabricator via cfe-commits]

varjujan updated this revision to Diff 82859.
varjujan added a comment.
Herald added subscribers: JDevlieghere, mgorny.

I have implemented some more heuristics to achieve better results.


https://reviews.llvm.org/D20689

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/SuspiciousCallArgumentCheck.cpp
  clang-tidy/misc/SuspiciousCallArgumentCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-suspicious-call-argument.rst
  test/clang-tidy/misc-suspicious-call-argument.cpp

Index: test/clang-tidy/misc-suspicious-call-argument.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-call-argument.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s misc-suspicious-call-argument %t -- -- -std=c++11
+
+void foo_1(int aa, int bb) { }
+
+void foo_2(int source, int aa) { }
+
+void foo_3(int valToRet, int aa) { }
+
+void foo_4(int pointer, int aa) { }
+
+void foo_5(int aa, int bb, int cc, ...) { }
+
+void foo_6(const int dd, bool& ee) { }
+
+class TestClass {
+public:
+  void thisFunction(int integerParam, int thisIsPARAM) {}
+};
+
+int main() {
+
+	// Equality test.
+  int aa, cc = 0;  
+  foo_1(cc, aa); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (aa) is swapped with aa (bb). [misc-suspicious-call-argument]  
+
+  // Abbreviation test.
+  int src = 0;
+  foo_2(aa, src); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: aa (source) is swapped with src (aa). [misc-suspicious-call-argument]
+  
+  // Levenshtein test.
+  int bb = 0;
+  foo_1(cc, bb); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (aa) is swapped with bb (bb). [misc-suspicious-call-argument]
+  
+  // Prefix test.
+  int  = 0;
+  foo_1(cc, ); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (aa) is swapped with  (bb). [misc-suspicious-call-argument]
+  
+  // Suffix test.
+  int urce = 0;
+  foo_2(cc, urce); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (source) is swapped with urce (aa). [misc-suspicious-call-argument]
+  
+  // Substring test.
+  int ourc = 0;
+  foo_2(cc, ourc); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (source) is swapped with ourc (aa). [misc-suspicious-call-argument]
+  
+  // Jaro-Winkler test.
+  int iPonter = 0;
+  foo_4(cc, iPonter); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (pointer) is swapped with iPonter (aa). [misc-suspicious-call-argument]
+  
+  // Dice test.
+  int aaabaa = 0;
+  foo_1(cc, aaabaa); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (aa) is swapped with aaabaa (bb). [misc-suspicious-call-argument]
+   
+  // Variadic function test.
+  int bb = 0;
+  foo_5(src, bb, cc, aa); // Should pass.
+  foo_5(cc, bb, aa, src); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: cc (aa) is swapped with aa (cc). [misc-suspicious-call-argument]
+  
+  //Type match
+  bool dd = false;
+  int ee = 0;
+  auto szam = 0;
+  foo_6(ee, dd); // Should pass.
+  foo_1(szam, aa); // Should fail.
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: szam (aa) is swapped with aa (bb). [misc-suspicious-call-argument]
+  
+  // Test lambda.
+  auto testMethod = [&](int method, int randomParam) { return 0; };
+  int method = 0;
+  testMethod(method, 0); // Should pass.
+
+  // Member function test
+  TestClass test;
+  int integ, thisIsAnArg = 0;
+  test.thisFunction(integ, thisIsAnArg); // Should pass.
+
+  return 0;
+}
Index: docs/clang-tidy/checks/misc-suspicious-call-argument.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-call-argument.rst
@@ -0,0 +1,9 @@
+misc-suspicious-call-argument
+=
+
+This checker finds those function calls where the function arguments are 
+provided in an incorrect order. It compares the name of the given variable 
+to the argument name in the function definition.
+
+It issues a message if the given variable name is similar to an another 
+function argument in a function call. It uses case insensitive comparison.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -81,6 +81,7 @@
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
+   misc-suspicious-call-argument
misc-suspicious-missing-comma
misc-suspicious-semicolon
misc-suspicious-string-compare
Index: clang-tidy/misc/SuspiciousCallArgumentCheck.h
==

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Varju Janos via Phabricator via cfe-commits]

varjujan added a comment.

I ran the check on multiple projects and tried to categorize the warnings: real 
errors, false positives, naming errors and coincidences. The results are 
attached. I got no warnings on LLVM.

F2902037: postgres 

F2902036: linuxKernel 

F2902035: xerces 

F2902034: libreOffice 


https://reviews.llvm.org/D20689



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20689: [clang-tidy] Suspicious Call Argument checker

[Varju Janos via Phabricator via cfe-commits]

varjujan added a comment.

@xazax.hun
Yes I do. Obviously some of them seem to be better than the others so I can 
remove a couple if needed.


https://reviews.llvm.org/D20689



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Varju Janos via Phabricator via cfe-commits]

varjujan added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:51
+{true, 40, 50}, // Substring.
+{true, 50, 66}, // Levenshtein.
+{true, 75, 85}, // Jaro-Winkler.

whisperity wrote:
> aaron.ballman wrote:
> > We should probably document where all these numbers come from, but `66` 
> > definitely jumps out at me as being a bit strange. :-D
> Unfortunately, I have absolutely no idea. All these values are percentages 
> between 0 and 100 (`-1` is just saying that //"This heuristic doesn't accept 
> percentages"//), and this is written in the documentation now. However, the 
> answer to //"**why** 66%?"//, unless @varjujan can say something, I think is 
> lost to history...
> 
> I'll read over his thesis once again, maybe I can find anything with regards 
> to this.
> 
> Either way, I've detailed from both the code and the thesis how the 
> percentages are meant. In some cases, the % is calculated as //"% of the 
> longer string's length"//. In the Leventhstein's case, it's actually inverted:
> 
> ```
> Dist = (1 - Dist / LongerLength) * 100;
> ```
> 
> So what this says is that if the current arg1-param1 arg2-param2 pairing has 
> less than the inverse of 50% (which is more than 50%) of the longer string's 
> edit distance, but the arg2-param1 and arg1-param2 (the suggested swapped 
> order) has more than the inverse of 66% (which is less than 33%), then the 
> swap will be suggested.
> 
> Originally these values were called `LowerBound` and `UpperBound`, 
> respectively, which was saying **even less** about what they mean...
Sadly, I think there isn't any scientific reason behind these numbers. They 
just looked ok after a couple of test runs. (Maybe they make sense for shorter 
arg names, like the 66 for 3 char long names.)


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