On Jul 2, 2012, at 3:04 PM, Dmitri Gribenko <[email protected]> wrote:
> On Mon, Jul 2, 2012 at 9:39 AM, Douglas Gregor <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
