[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

This revision was automatically updated to reflect the committed changes. Closed by commit rL299653: [analyzer] Reland r299544 "Add a modular constraint system to the CloneDetector" (authored by dergachev). Changed prior to commit: https://reviews.llvm.org/D23418?vs=94337=94370#toc

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor added a comment. I think for range loops work differently: #include #include #include struct Foo { int* begin() const { assert(false); } int* end() const { assert(false); } void bar() const { std::cout << "Different behavior" << std::endl; } }; int main() { std::vector

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ added a comment. $ cat test.c #include int main() { int i = 5; { int i = i; printf("%d\n", i); } return 0; } $ clang test.c $ ./a.out 32767 $ clang test.c -Xclang -ast-dump ... `-FunctionDecl 0x7ff82d8eac78 line:2:5 main

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor added inline comments. Comment at: lib/Analysis/CloneDetection.cpp:395 + + for (const Stmt *Child : S->children()) { +if (Child == nullptr) { I couldn't reproduce, but from what I assume form the warning and the crash that we confused the

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 94337. teemperor added a comment. - Renamed variable from 'S' to 'Child' to make the buildbots happy (and because it's more expressive) - Fixed the name of the unit test. https://reviews.llvm.org/D23418 Files: include/clang/Analysis/CloneDetection.h

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

spatel added a comment. In https://reviews.llvm.org/D23418#719139, @NoQ wrote: > Hmm, reverted because i'm seeing crashes on some buildbots (works for me > though). > > It's crashing somewhere in `saveHash`, seems that some `Stmt`s are null. For > instance, >

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ reopened this revision. NoQ added a comment. This revision is now accepted and ready to land. Hmm, reverted because i'm seeing crashes on some buildbots (works for me though). It's crashing somewhere in `saveHash`, seems that some `Stmt`s are null. For instance,

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

This revision was automatically updated to reflect the committed changes. Closed by commit rL299544: [analyzer] Add a modular constraint system to the CloneDetector (authored by dergachev). Changed prior to commit: https://reviews.llvm.org/D23418?vs=93503=94227#toc Repository: rL LLVM

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor added a comment. Change it to a namespace when merging if you prefer that :). Thanks for the review! https://reviews.llvm.org/D23418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ accepted this revision. NoQ added a comment. Yay, i think this is good to go. Sorry again for being slow>< I guess i'd go ahead and land this patch soon. Comment at: include/clang/Analysis/CloneDetection.h:228 +/// custom constraints. +class CloneConstraint { +public:

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

xiangzhai added a comment. Hi Raphael, Then I will rebase my patch https://reviews.llvm.org/D31320 please review it when you have free time, thanks a lot! Regards, Leslie Zhai https://reviews.llvm.org/D23418 ___ cfe-commits mailing list

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 93503. teemperor added a comment. - Remove the mysterious unicode-character at the start of CloneDetection.cpp - Fixed formatting of the comment in CloneDetectionTest.cpp - Fixed comment in StmtSequence::contains that was still talking about checking for

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

xiangzhai added a comment. Hi Raphael, Congratulate the patch was accepted! > Well this patch won't change a lot with the false-positives or performance > (it's more refactoring) :) So may I Teach CloneDetection about Qt Meta-Object Compiler and filter other

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

v.g.vassilev accepted this revision. v.g.vassilev added a comment. This revision is now accepted and ready to land. I am very happy to see this converging! Thanks for the work and perseverance! LGTM. https://reviews.llvm.org/D23418 ___ cfe-commits

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 93302. teemperor added a comment. - `auto` to `const auto` to be more consistent with LLVM style. https://reviews.llvm.org/D23418 Files: include/clang/Analysis/CloneDetection.h lib/Analysis/CloneDetection.cpp

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 93270. teemperor added a comment. - Remove the `assert(ChildHash)` which is obviously wrong code that just rarely fails. https://reviews.llvm.org/D23418 Files: include/clang/Analysis/CloneDetection.h lib/Analysis/CloneDetection.cpp

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor marked an inline comment as done. teemperor added a comment. Well this patch won't change a lot with the false-positives or performance (it's more refactoring) :) https://reviews.llvm.org/D23418 ___ cfe-commits mailing list

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

xiangzhai added a comment. Hi Raphael, Thanks for your reply! > regarding performance: Last time I checked we spend most of the time on the > verification of the hash values. We can do some tricks to make this faster > (like delaying the verification to the end of the constraints where we

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor marked 7 inline comments as done. teemperor added a comment. Hey Leslie, regarding performance: Last time I checked we spend most of the time on the verification of the hash values. We can do some tricks to make this faster (like delaying the verification to the end of the

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 93180. teemperor added a comment. Herald added a subscriber: mgorny. Thanks for the review Artem! Changes: - No longer including the old LLVM hashing header. - Fixed the messed up comment formatting when i removed all the `\brief`s... - `CloneConstraint`

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

xiangzhai added a comment. Hi Raphael, Thanks for your awesome job! I have read about the slide of "LLVM: built-in scalable code clone detection based on semantic analysis", and sent email to Shamil for asking "Is Code clone

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

v.g.vassilev added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:147 +// different algorithm). +// TODO: In very big clones even multiple variables can be unintended, +// so replacing this number with a percentage could better

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ added a comment. I think this is awesome. Would anybody like to move concrete constraint class definitions to a separate header + translation unit, so that to highlight how simple the primary interface is to a person who haven't gone through all the discussion? Comment

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

v.g.vassilev added a comment. We were planning adding some extra unittests, too... https://reviews.llvm.org/D23418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ added a comment. I'm sorry for the delays; i'm humbly requesting a couple more weeks to get back to this awesome stuff. https://reviews.llvm.org/D23418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated this revision to Diff 89084. teemperor added a comment. - Removed all the deprecated `\brief`s I couldn't find any actual regression in the code now, so from my side it's ok to merge it. https://reviews.llvm.org/D23418 Files: include/clang/Analysis/CloneDetection.h

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor updated the summary for this revision. teemperor updated this revision to Diff 77759. teemperor added a comment. - Rebased patch to the current trunk state. - Replaced runtime polymorphism with templates. - Constraint interface now only has one method signature.

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor planned changes to this revision. teemperor added a comment. - I ran all real-world tests (sqlite, etc.) before rebasing to trunk. I'm not 100% confident that I correctly merged everything, so I'll rerun them just in case. The normal clang test-suite passes, so it looks good.

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

mehdi_amini added a comment. (Quick stylistic comments) Comment at: include/clang/Analysis/CloneDetection.h:224 - /// \brief Searches the provided statements for clones. + /// \brief Represents a way to filter clones from a list of clone groups. /// Use

Re: [PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ added a comment. Here's some pseudo-code of the way i see it. // This interface mimics CloneDetector's interface, hence omnipotent but useless. class BasicConstraint { public: virtual void add(const StmtSequence ) = 0; virtual vector findClones() = 0; }; // This

Re: [PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

NoQ added inline comments. Comment at: include/clang/Analysis/CloneDetection.h:243 @@ +242,3 @@ +/// clone groups from the given hash group. +virtual bool acceptsHashGroup(const CloneGroup ); + I might be wishing a lot, but i've a feeling this

Re: [PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

v.g.vassilev added inline comments. Comment at: include/clang/Analysis/CloneDetection.h:235 @@ +234,3 @@ + /// respective virtual methods. + class Constraint { + public: I am still unsure about the name. It is too generic. What about `CloneConstraint`?

Re: [PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

teemperor retitled this revision from "[analyzer] Added a pass architecture to the CloneDetector" to "[analyzer] Added a reusable constraint system to the CloneDetector". teemperor updated the summary for this revision. teemperor updated this revision to Diff 68680. teemperor added a comment. -