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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
