[cfe-commits] [PATCH] Fix binding of nodes in case of forEach..() matchers.

[Daniel Jasper]

================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:59
@@ -58,3 +58,3 @@
 visitMatchesRecursively(Visitor* ResultVisitor,
-                        BoundNodesMap* AggregatedBindings) {
-  Bindings.copyTo(AggregatedBindings);
+                        const BoundNodesMap& AggregatedBindings) {
+  BoundNodesMap CombinedBindings;
----------------
Manuel Klimek wrote:
> Why can't we pass by-value here?
We could. I wanted to make it very explicit that copying is going on here. That might have prevented the previous regression.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.h:41
@@ -40,3 +40,3 @@
     if (FindResultReviewer != NULL) {
-      *Verified = FindResultReviewer->run(&Result.Nodes, Result.Context);
+      *Verified |= FindResultReviewer->run(&Result.Nodes, Result.Context);
     } else {
----------------
Manuel Klimek wrote:
> How sure are you that there are no tests that rely on this to catch regressions?
If a test exists, it is a bad test that might break for the wrong reasons:
- With this change, the cases with and without a BoundNodesCallback are consistent (i.e. Verified gets set to true if at least one right match is found).
- Without this, it relies on ordering. Verified will always be set to the whether the BoundNodesCallback accepts the last occurring match). So changing the ordering of the input source code can have weird effects. And as far as I know, we so far have not introduced guarantees on the ordering of matches.

That being said, I am convinced that no test is relying on this on purpose.


http://llvm-reviews.chandlerc.com/D112