[cfe-commits] [PATCH] TableGen'ize documentation command lists (and break binary compatibility of CXComment)

[Douglas Gregor]

On Sep 8, 2012, at 4:12 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Wed, Sep 5, 2012 at 10:11 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> Hello,
>> 
>> Doug: CCing you since this email is mostly about libclang CXComment
>> binary compatibility I need to break for a good reason.
>> 
>> The attached patch TableGen'izes all command lists in
>> CommentCommandTraits.cpp and now we have a list of all commands.  This
>> is a good thing in itself, but it also enables us to easily implement
>> typo correction for command names.
>> 
>> With this change we have objects that contain information about each
>> command, so it makes sense to resolve command name just once during
>> lexing (currently we store command names as strings and do a linear
>> search every time some property value is needed).  Thus comment token
>> and AST nodes were changed to contain a command ID -- index into a
>> tables of builtin and registered commands.  Unknown commands are
>> registered during parsing and thus are also uniformly assigned an ID.
>> Using an ID instead of a StringRef is also a nice memory optimization
>> since ID is a small integer that fits into a common bitfield in
>> Comment class.

I like this a lot better!

>> This change implies that to get any information about a command (even
>> a command name) we need a CommandTraits object to resolve the command
>> ID to CommandInfo*.  Currently a fresh temporary CommandTraits object
>> is created whenever it is needed since it does not have any state.
>> But with this change it has state -- new commands can be registered,
>> so I added a CommandTraits object to ASTContext.

Okay. I can certainly imagine that tools would want to add their own Doxygen-style commands and then pull them out of comments, since it's a nice extra-linguistic way to get information to the tool without getting in the way.

>> Also, in libclang CXComment has to be expanded to include a pointer to
>> ASTContext so that all functions working on comment AST nodes can get
>> a CommandTraits object.  This breaks binary compatibility of CXComment
>> APIs.
>> 
>> We can avoid breaking binary compatibility by storing CommandInfo* in
>> AST nodes instead of unsigned command IDs (that need to be resolved).
>> But with this approach we won't get that nice memory optimization.

It's fine to break binary compatibility of the CXComment APIs. They haven't made it into a release.

>> The patch is long, but changes are mostly boring and just change the
>> code to use the new APIs to access command properties.  It is the
>> architecture and binary compatibility I want to get comments about.
> 
> I was reviewing this thread and noticed that I attached the wrong
> version of the patch -- sorry for that.  This is the correct one
> (still based on the same approach -- CXComment is a value type).


Index: include/clang-c/Index.h
===================================================================
--- include/clang-c/Index.h	(revision 163467)
+++ include/clang-c/Index.h	(working copy)
@@ -2040,7 +2040,8 @@
  * \brief A comment AST node.
  */
 typedef struct {
-  const void *Data;
+  const void *ASTNode;
+  void *ASTContext;
 } CXComment;

How about storing the CXTranslationUnit rather than the ASTContext?

+/// This class provides informaiton about commands that can be used
+/// in comments.

Typo "informaiton". 

+class CommandTraits {
+public:

How about making this noncopyable?

This looks great, thanks!

	- Doug