[cfe-commits] Comment AST and parser

[Douglas Gregor]

On Jul 2, 2012, at 3:04 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Mon, Jul 2, 2012 at 9:39 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> This is looking great! I like this direction a lot. Some specific comments below.
> 
> Thank you for looking!  Answers to discussion questions follow.
> 
>> +/// A newline.
>> +class NewlineComment : public InlineContentComment {
>> +public:
>> 
>> Why have a special AST node for newlines, rather than either embedding \n's or having a "there's a newline following this inline comment" bit? Abstractly, I guess having an AST node to cope with might make it easier for clients of the comment AST to translate it properly, or is there another reason?
> 
> Information about newlines can be used to correctly display HTML tag
> <pre>.  (But we have verbatim blocks, so why use <pre>?)  I don't know
> if it will be useful for anything except that.  The parser can just
> silently drop newlines.   I thought that it would be easier to remove
> newline nodes if we consider them useless than to introduce them --
> that's the only reason I added this AST node.
> 
> Turning a newline node into a bit on inline content seems a good idea,
> except that it makes all inline content nodes larger…

There are free bits in the Comment base class you could use, which makes the bit essentially free (and eliminates the overhead of the NewlineComment nodes).


>> Index: utils/TableGen/TableGen.cpp
>> ===================================================================
>> --- utils/TableGen/TableGen.cpp (revision 159471)
>> +++ utils/TableGen/TableGen.cpp (working copy)
>> @@ -38,6 +38,7 @@
>>    GenClangDiagsDefs,
>>    GenClangDiagGroups,
>>    GenClangDiagsIndexName,
>> +  GenClangCommentNodes,
>>    GenClangDeclNodes,
>>    GenClangStmtNodes,
>>    GenClangSACheckers,
>> @@ -86,6 +87,8 @@
>>                      clEnumValN(GenClangDiagsIndexName,
>>                                 "gen-clang-diags-index-name",
>>                                 "Generate Clang diagnostic name index"),
>> +                    clEnumValN(GenClangCommentNodes, "gen-clang-comment-nodes",
>> +                               "Generate Clang AST comment nodes"),
>>                      clEnumValN(GenClangDeclNodes, "gen-clang-decl-nodes",
>>                                 "Generate Clang AST declaration nodes"),
>>                      clEnumValN(GenClangStmtNodes, "gen-clang-stmt-nodes",
>> @@ -148,6 +151,9 @@
>>      case GenClangDiagsIndexName:
>>        EmitClangDiagsIndexName(Records, OS);
>>        break;
>> +    case GenClangCommentNodes:
>> +      EmitClangASTNodes(Records, OS, "Comment", "");
>> +      break;
>>      case GenClangDeclNodes:
>>        EmitClangASTNodes(Records, OS, "Decl", "Decl");
>>        EmitClangDeclContext(Records, OS);
>> 
>> Is there more that you intend to do with TableGen for the AST nodes themselves? It looks like all we're getting is the list of node names, but TableGen is a pretty heavyweight way to keep that up-to-date.
> 
> Currently we tablegen'ing clang/AST/CommentNodes.inc.  I plan to
> tablegen a list of supported commands with two attributes: command
> kind (inline/block/verbatim block/verbatim line) and number of
> arguments.

It seems to me like this will be a different set of TableGen classes with different generators. If that's indeed true, using TableGen to generate CommentNodes.inc is rather excessive. A .def file would be simpler in such cases.

	- Doug