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

[Chandler Carruth]

On Tue, Oct 2, 2012 at 9:22 AM, Alexander Kornienko <alexfh at google.com>wrote:

> On Tue, Oct 2, 2012 at 5:49 PM, 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.
>
>
> Let's put it this way: having a way to use "previous line" or "current
> line + offset" variable in FileCheck, and having a better FileCheck
> performance in debug mode, what would you prefer?
>
> TEST(StmtDumper, TestDeclStmt) {
>   ASSERT_TRUE(DumpedStmtCXX98Matches(
>     "void A() {\n"
>     "  int a = 1, b;\n"
>     "}",
>     "A",
>     "(DeclStmt 0xXXXXXXXX <input.cc:2:3, col:15>\n"
>     "  (VarDecl 0xXXXXXXXX <col:3, col:11> a 'int'\n"
>     "    (IntegerLiteral 0xXXXXXXXX <col:11> 'int' 1))\n"
>     "  (VarDecl 0xXXXXXXXX <col:3, col:14> b 'int'))\n"));
> }
>
> or
>
> // RUN: clang_cc1 -ast-dump --std=c++98 -DTEST33 %s | FileCheck
> --strict-whitespace --check-prefix CHECK33 %s
> #ifdef TEST33
> void A() {
> // CHECK33: {{^}}(DeclStmt 0x{{........}}
> <input.cc:[[__LINE_MINUS_1__]]:3, col:15>
>   int a = 1, b;
> // CHECK33-NEXT: {{^  }}(VarDecl 0x{{........}} <col:3, col:11> a 'int'
> {{$}}
> // CHECK33-NEXT: {{^    }}(IntegerLiteral 0x{{........}} <col:11> 'int' 1))
> {{$}}
> // CHECK33-NEXT: {{^  }}(VarDecl 0x{{........}} <col:3, col:14> b
> 'int')){{$}}
> }
> #endif
>
> These both seem ugly to some extent, both use some form of escaping either
> for code or for check patterns. But the second option has less
> dependencies, everything is in one place (including compiler arguments),
> there's no need to include any headers here, no need to use any additional
> libraries, no compilation required prior to running the test, you can play
> with it using clang alone, and check it with FileCheck if needed. And the
> most important thing for me is that it doesn't have its own pattern
> matching rules, but reuses the tool already widely known and used in llvm.
> The first option is a bit less verbose, but it uses custom infrastructure,
> that has no other use-cases except tests for dump stuff.
>

 You've almost sold me, personally, but the leading whitespace stuff is
nuts.

Have folks looked into using an output format that makes the indentation
easier to check? It might also make it easier for humans to read...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121002/5b760969/attachment.html>