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

[Philip Craig]

================
Comment at: unittests/AST/SourceLocationTest.cpp:29
@@ +28,3 @@
+public:
+  MatchVerifier() : ExpectId("") {}
+
----------------
Manuel Klimek wrote:
> ExpectId seems completely unused now...
Yes, I left it there in case a test needs to specify its own bind within the matcher. I can delete it for now if you think that's better.

================
Comment at: unittests/AST/SourceLocationTest.cpp:95
@@ +94,3 @@
+  if (!Node) {
+    setResult("Could not find node with id \"" + ExpectId + "\"");
+    Verified = false;
----------------
Manuel Klimek wrote:
> Can we instead use EXPECT(...) to fail the test? That way googletest would take care of keeping track of errors etc.
This result is returned at the end of match(), which is used in an EXPECT(), so it will keep track of the error.

================
Comment at: unittests/AST/SourceLocationTest.cpp:186
@@ +185,3 @@
+TEST(SourceLocation, KNRParamLocation) {
+  LocationVerifier<ParmVarDecl> Verifier;
+  Verifier.expectLocation(1, 8).expectId("i");
----------------
Manuel Klimek wrote:
> Philip Craig wrote:
> > Manuel Klimek wrote:
> > > 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...
> > I didn't put it in the constructor because I expect LocationVerifier and RangeVerifier to be inherited from for testing different location members of nodes, and I didn't want the derived classes to need to define constructors and forward the arguments up. Is there a way to avoid that?
> I would expect the constructors to then take different arguments. Also, I think the price of having a little more boilerplate is fine for the clearer use... I don't feel super-strongly about this, though, so feel free to make the call either way.
I'll add some test cases that need to use different verifiers and see how it looks after that.


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