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