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

[Manuel Klimek]

================
Comment at: unittests/AST/SourceLocationTest.cpp:24
@@ +23,3 @@
+/// \brief Base class for verifying some property of nodes found by a matcher.
+class MatchVerifier : public MatchFinder::MatchCallback {
+public:
----------------
Philip Craig wrote:
> Manuel Klimek wrote:
> > I see overlap in the implementation with both TestVisitor.h and RecursiveASTVisitorTest.cpp (which already has location testing).
> > 
> > Do you think we can pull out something common?
> There's bits of overlap between all of these unit tests, and I'm not sure yet which bits should be common.
> 
> I started using TestVisitor.h, but the structure of that code doesn't lend itself well to adding in range checking. I couldn't see a way to add it without either rewriting it all, or duplicating a lot of code.
> 
> The secondary issue is that TestVisitor.h doesn't use ASTMatchers, and I think they are quite useful for this sort of testing. I'm assuming that ASTMatchers shouldn't be used for testing RAV, since it's essentially another lay on top of RAV, so there may be things you can't test that way.
Yes, you're right. My main concern was that I wanted to make sure you know of the other instances, and didn't see a quick way to integrate them. I can poke at that later...

================
Comment at: unittests/AST/SourceLocationTest.cpp:186
@@ +185,3 @@
+TEST(SourceLocation, KNRParamLocation) {
+  LocationVerifier<ParmVarDecl> Verifier;
+  Verifier.expectLocation(1, 8).expectId("i");
----------------
Both here and below having the extra expactLocation seems superfluous, if we do not also want to support multiple locations (which probably would require a slightly different interface again).

I would just put the stuff into the constructor. Unless you have plans for adding much more to the interface...


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