[cfe-commits] [PATCH] Implement AST dumper for Decls

[Chandler Carruth]

On Tue, Oct 2, 2012 at 8:49 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Tue, Oct 2, 2012 at 6:46 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
> > On Tue, Oct 2, 2012 at 5:36 PM, Dmitri Gribenko <gribozavr at gmail.com>
> wrote:
> >>
> >> On Tue, Oct 2, 2012 at 6:28 PM, Alexander Kornienko <alexfh at google.com>
> >> wrote:
> >> > On Tue, Oct 2, 2012 at 5:15 PM, Dmitri Gribenko <gribozavr at gmail.com>
> >> > wrote:
> >> >>
> >> >> On Tue, Oct 2, 2012 at 5:59 PM, Alexander Kornienko <
> alexfh at google.com>
> >> >> wrote:
> >> >> > So I'm definitely for using FileCheck in this case, but it's only
> me,
> >> >> > others
> >> >> > may disagree.
> >> >>
> >> >> I'm in favor of ASTMatchers-based tests for anything dumping related,
> >> >> based on my experience of maintaining
> >> >> test/Index/annotate-comments.cpp.  Any small change to the testcases
> >> >> forces a change of line numbers for everything that follows it (half
> >> >> of tests, on average).
> >> >
> >> > FileCheck has variables now, maybe we can add a special __LINE__
> >> > variable
> >> > (possibly with a way to specify offset)? In this case we could put
> CHECK
> >> > lines near the actual test code and this would not be bound to
> absolute
> >> > line
> >> > numbers. What do you think?
> >>
> >> That would help somewhat.  But nevertheless, annotate-comments.cpp is
> >> the slowest testcase in the testsuite (for some reason, FileCheck
> >> takes up to 10s to match it in debug version -- spending most of the
> >> time in regex matching).
> >
> >
> > I'll look into adding special variables to FileCheck.
> >
> > As for debug build, can we somehow use release FileCheck even when
> testing a
> > debug configuration?
> >
> >>
> >> >> With ASTMatchers-based tests we can keep all
> >> >> logically related tests in a single file and keep the test case close
> >> >> to the expected output while ensuring that different tests are
> >> >> isolated from each other.
> >> >
> >> > Isolation is possible now using separate RUN lines with specific -D
> >> > defines
> >> > and #ifdef blocks.
> >>
> >> That's more like putting the isolation into a tool that didn't have
> >> it, versus having an approach where testcases are intrinsically
> >> isolated.
> >
> > My problem with ASTMatchers-based tests for dumping is that there are
> lots
> > of dependencies, that can be avoided. And the test infrastructure (a
> custom
> > test infrastructure, used only for one test) is an additional possible
> point
> > of failure, and definitely requires some support. In my opinion it makes
> > sense only in case this infrastructure is generalized, moved elsewhere
> and
> > gets its own tests. But still this seems to be overkill.
>
> The infrastructure is rather simple in my opinion, and can (should!)
> be shared between (Decl|Stmt)(Printer|Dumper) tests.


So, generally I do agree. However, FileCheck was *designed* for testing
textual output. It seems a bit weird to not be using it here...

I wonder if it would make sense to hoist the FileCheck logic into a library
such that the unittests could actually use the same syntax for making
textual assertions from within unittests? This has been my biggest
frustration with unittests checking textual output -- often the errors do
not make it clear what broke or how to fix. They also tend to devolve to
golden-output tests rather than using minimal assertions to check for the
important output.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121002/8b7b7b7d/attachment.html>