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

[Philip Craig]

================
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:
----------------
Chandler Carruth wrote:
> 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.
Added FIXMEs.

================
Comment at: unittests/AST/SourceLocationTest.cpp:118-120
@@ +117,5 @@
+      this->setFailure(
+          "Expected location <" +
+          Twine(ExpectLine) + ":" + Twine(ExpectColumn) + ">, found <" +
+          Twine(Line) + ":" + Twine(Column) + ">");
+    }
----------------
Chandler Carruth wrote:
> 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.
Done, although I couldn't see an equivalent way of printing ranges.


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