================
Comment at: unittests/AST/SourceLocationTest.cpp:29
@@ +28,3 @@
+public:
+ MatchVerifier() : ExpectId("") {}
+
----------------
ExpectId seems completely unused now...
================
Comment at: unittests/AST/SourceLocationTest.cpp:95
@@ +94,3 @@
+ if (!Node) {
+ setResult("Could not find node with id \"" + ExpectId + "\"");
+ Verified = false;
----------------
Can we instead use EXPECT(...) to fail the test? That way googletest would take
care of keeping track of errors etc.
================
Comment at: unittests/AST/SourceLocationTest.cpp:186
@@ +185,3 @@
+TEST(SourceLocation, KNRParamLocation) {
+ LocationVerifier<ParmVarDecl> Verifier;
+ Verifier.expectLocation(1, 8).expectId("i");
----------------
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.
http://llvm-reviews.chandlerc.com/D72
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
