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

Philip Craig philipjcraig at gmail.com
Wed Oct 24 02:17:40 PDT 2012



================
Comment at: unittests/AST/SourceLocationTest.cpp:29
@@ +28,3 @@
+public:
+  MatchVerifier() : ExpectId("") {}
+
----------------
Manuel Klimek wrote:
> Philip Craig wrote:
> > 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.
> I would delete it. I dislike unused things that "might" be used in the future, as more often than not they end up staying unused.
Fixed.

================
Comment at: unittests/AST/SourceLocationTest.cpp:95
@@ +94,3 @@
+  if (!Node) {
+    setResult("Could not find node with id \"" + ExpectId + "\"");
+    Verified = false;
----------------
Manuel Klimek wrote:
> Philip Craig wrote:
> > 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.
> I'm aware - my point is that all the result tracking could be deleted and replaced with one EXPECT here, which would lead to simpler code overall. Instead of EXPECT_TRUE(match(...)), the test could call Verifier.expectMatch(...).
Doing that means that failures no longer print the line number of the test case that is failing, but in most cases it will be the test case that needs fixing, not the run() function. You still have the test name, but it's easier to navigate to a line number. If that is acceptable, then this certainly simplifies things a lot.


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



More information about the cfe-commits mailing list