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

[Kirill Bobyrev via cfe-commits]

omtcyfz added inline comments.

================
Comment at: lib/Analysis/CloneDetection.cpp:436
@@ +435,3 @@
+    if (IsInMacro) {
+      Signature.Complexity = 0;
+    }
----------------
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.


https://reviews.llvm.org/D23316