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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
