[cfe-commits] r158771 - in /cfe/trunk: include/clang-c/ include/clang/AST/ include/clang/Basic/ include/clang/Comments/ include/clang/Lex/ include/clang/Parse/ include/clang/Sema/ include/clang/Serialization/ lib/ lib/ARCMigrate/ lib/AST/ lib/Com

[Douglas Gregor]

On Jun 20, 2012, at 2:40 AM, Chandler Carruth <chandlerc at google.com> wrote:

> On Tue, Jun 19, 2012 at 5:34 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
> Author: gribozavr
> Date: Tue Jun 19 19:34:58 2012
> New Revision: 158771
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=158771&view=rev
> Log:
> Structured comment parsing, first step.
> 
> I'm pretty excited to see this coming along, but...
>  
> 
> * Retain comments in the AST
> * Serialize/deserialize comments
> * Find comments attached to a certain Decl
> * Expose raw comment text and SourceRange via libclang
> 
> Added:
>    cfe/trunk/include/clang/Comments/
>    cfe/trunk/include/clang/Comments/RawCommentList.h
>    cfe/trunk/lib/Comments/
>    cfe/trunk/lib/Comments/CMakeLists.txt
>    cfe/trunk/lib/Comments/Makefile
>    cfe/trunk/lib/Comments/RawCommentList.cpp
> 
> Why are we introducing an entire new library just for one class with one source file?
> 
> This doesn't work at all -- this library depends on the AST library, and yet the AST library uses classes defined in this library. It's a bad layering violation, and it's not at all clear to me why it is needed.
> 
> I could see wanting the RawCommentList to be a library *below* the AST library, and if so I would expect it to be part of the Lex library with the preprocessor or the Basic library where the SourceManager itself is... I'm not really enthusiastic about growing an entire separate library just for this construct.

The expectation is that the comment parsing is going to grow a number of related data structures that express the parsed, structured comments. This will essentially be an AST of the comments themselves, describing (e.g.,) the grouping structure of comments, and that feels like it might be too large to introduce into the AST library proper. That said, it's not at all clear how closely tied this structured comment AST will be to the actual AST; we may be forced into pushing all of that into the AST library due to circular references.

> Anyways, the layering violation and circular dependency is a serious problem and was breaking some builds so I've folded this code into the AST library. You can sink it or move it around based on this discussion.


Thanks for cleaning up the glaring layering violation.

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120620/67417a66/attachment.html>