On Sep 8, 2012, at 4:12 AM, Dmitri Gribenko <[email protected]> wrote:
> On Wed, Sep 5, 2012 at 10:11 AM, Dmitri Gribenko <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
