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

Philip Craig philipjcraig at gmail.com
Thu Nov 1 05:23:21 PDT 2012

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 {
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.


More information about the cfe-commits mailing list