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

[Matthieu Monrocq]

On Wed, Sep 5, 2012 at 9: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.
>
> 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.
>
> 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.
>

If you need to break binary compatibility, I would encourage you to go all
the way and make CXComment an opaque pointer. This way further changes will
never again require breaking binary compatibility (for this).

-- Matthieu


>
> 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.
>
> 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.
>
> Please review.
>
> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120905/821cb801/attachment.html>