[PATCH] D23316: [analyzer] Fixed the false-positives caused by macro generated code.

[Raphael Isemann via cfe-commits]

teemperor marked 10 inline comments as done.

================
Comment at: lib/Analysis/CloneDetection.cpp:436
@@ +435,3 @@
+    if (IsInMacro) {
+      Signature.Complexity = 0;
+    }
----------------
NoQ wrote:
> omtcyfz wrote:
> > omtcyfz wrote:
> > > omtcyfz wrote:
> > > > NoQ wrote:
> > > > > omtcyfz wrote:
> > > > > > Do I understand correctly that a code generated by a macro doesn't affect "complexity" at all then?
> > > > > > 
> > > > > > ```
> > > > > > TEST_F(QueryParserTest, Complete) {
> > > > > >   std::vector<llvm::LineEditor::Completion> Comps =
> > > > > >       QueryParser::complete("", 0, QS);
> > > > > >   ASSERT_EQ(6u, Comps.size());
> > > > > >   EXPECT_EQ("help ", Comps[0].TypedText);
> > > > > >   EXPECT_EQ("help", Comps[0].DisplayText);
> > > > > >   EXPECT_EQ("let ", Comps[1].TypedText);
> > > > > >   EXPECT_EQ("let", Comps[1].DisplayText);
> > > > > >   EXPECT_EQ("match ", Comps[2].TypedText);
> > > > > >   EXPECT_EQ("match", Comps[2].DisplayText);
> > > > > >   EXPECT_EQ("set ", Comps[3].TypedText);
> > > > > >   EXPECT_EQ("set", Comps[3].DisplayText);
> > > > > >   EXPECT_EQ("unlet ", Comps[4].TypedText);
> > > > > >   EXPECT_EQ("unlet", Comps[4].DisplayText);
> > > > > >   EXPECT_EQ("quit", Comps[5].DisplayText);
> > > > > >   EXPECT_EQ("quit ", Comps[5].TypedText);
> > > > > > 
> > > > > >   Comps = QueryParser::complete("set o", 5, QS);
> > > > > >   ASSERT_EQ(1u, Comps.size());
> > > > > >   EXPECT_EQ("utput ", Comps[0].TypedText);
> > > > > >   EXPECT_EQ("output", Comps[0].DisplayText);
> > > > > > 
> > > > > >   Comps = QueryParser::complete("match while", 11, QS);
> > > > > >   ASSERT_EQ(1u, Comps.size());
> > > > > >   EXPECT_EQ("Stmt(", Comps[0].TypedText);
> > > > > >   EXPECT_EQ("Matcher<Stmt> whileStmt(Matcher<WhileStmt>...)",
> > > > > >             Comps[0].DisplayText);
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > This is an actual piece of code from `extra/unittests/clang-query/QueryParserTest.cpp`. Yes, it is a test, but it still is a nice example of how many macros can be found in code (especially if we are talking about pure C or some weird C++).
> > > > > > 
> > > > > > Thus, I think it is reasonable to treat macro invocation as a `1`-"complexity" node.
> > > > > This "0" is not for the macro itself, but for the statements into which it expands. Macro itself is not a statement. If we put "1" here, it would produce a lot more complexity than you want.
> > > > > 
> > > > > That said, it's a good idea to treat every macro as a "complexity-1" statement, just need to figure out how to implement that correctly :)
> > > > > 
> > > > > Perhaps scan the source range of the sequence for how many different macro expansions are included, and add that number to complexity(?)
> > > > > This "0" is not for the macro itself, but for the statements into which it expands. Macro itself is not a statement. If we put "1" here, it would produce a lot more complexity than you want.
> > > > 
> > > > Sure, I understand that, this is why I didn't suggest putting `1` there.
> > > > 
> > > > > Perhaps scan the source range of the sequence for how many different macro expansions are included, and add that number to complexity(?)
> > > > 
> > > > Yes, this is exactly the solution that would work. Since macros aren't in the AST we'd need to get through SourceRange anyway.
> > > Though, it has to be optimized in order to prevent parsing a SourceLocation multiple times.
> > *visiting each SourceLocation
> Yeah, as a rough approximation we could count macro expansions within the current statement's children...
I'm now checking all expanded macros of the start/end locations. This should handle everything if I see that correctly (beside empty non-function macros which I marked as false-positives - not sure how we best handle them).

================
Comment at: test/Analysis/copypaste/macros.cpp:11
@@ +10,3 @@
+int max(int a, int b) { // expected-warning{{Detected code clone.}}
+  return a > b ? a : b;
+}
----------------
v.g.vassilev wrote:
> Wouldn't it be a good idea to have a fixit hint, saying "Did you mean to use ABS(a,b)". If the suggestion is applied, it would make the code more consistent, however it would encourage using preprocessor tricks (which is not always considered as good practice).
I don't think detecting clones between macro definitions and normal code is easily possible. Doing the same for functions however is certainly possible (i.e. "did you meant to call `max(a, b)`). I added that to the open points list.


https://reviews.llvm.org/D23316