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

Reply via email to