[cfe-commits] [PATCH] Add unit tests for source locations of AST nodes

[Chandler Carruth]

  Sure, here are my thoughts.

  I'm generally fine with you committing this Manuel whenever, as long as someone does the follow-ups to restructure it. If you're not likely to have time to do the follow-ups, then we might want to wait to commit until it's a bit closer to the final shape of things.


================
Comment at: unittests/AST/SourceLocationTest.cpp:25-27
@@ +24,5 @@
+
+/// \brief Base class for verifying some property of nodes found by a matcher.
+template <typename NodeType>
+class MatchVerifier : public MatchFinder::MatchCallback {
+public:
----------------
Add a FIXME to hoist this into a general testing library that can be shared between different AST tests? Also, when that happens, we should really consider making these GoogleMock matchers, as controversial as that may be.

An important note is that I suspect that long term we will want to slice the AST unit tests differently. It seems better long-term to have a unit test file for an AST node (or group of related nodes) rather than a test for source locations for all AST nodes.

I'm fine with starting here though, as these are the types of things that are most pressing. I would just like to have some of the forward directions charted out in comments for future maintainers.

Some of this should probably go in the top-level file comment.

I'll try to chat w/ dgregor, efriedma and others about the exact long-term design of test for the AST.

================
Comment at: unittests/AST/SourceLocationTest.cpp:118-120
@@ +117,5 @@
+      this->setFailure(
+          "Expected location <" +
+          Twine(ExpectLine) + ":" + Twine(ExpectColumn) + ">, found <" +
+          Twine(Line) + ":" + Twine(Column) + ">");
+    }
----------------
It would be better in all of these printing contexts to re-use the existing printing facilities.

Specifically, FullSourceLoc should be avoided here. You're not carrying these things around or storing them for a long time. The only reason for FullSourceLoc to exist is when it is inconvenient to mention the source manager, and it isn't here.

Instead, use SourceLocation, and use SourceLocation::print to render them as text.


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