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

[Manuel Klimek]

================
Comment at: unittests/AST/SourceLocationTest.cpp:29
@@ +28,3 @@
+public:
+  MatchVerifier() : ExpectId("") {}
+
----------------
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.

================
Comment at: unittests/AST/SourceLocationTest.cpp:95
@@ +94,3 @@
+  if (!Node) {
+    setResult("Could not find node with id \"" + ExpectId + "\"");
+    Verified = false;
----------------
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(...).


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