[cfe-dev] [RFC] Removing libclang APIs to traverse the comment AST

[Sean Silva]

On Wed, Nov 13, 2013 at 8:08 PM, Dmitri Gribenko <gribozavr at gmail.com>wrote:

> On Wed, Nov 13, 2013 at 4:51 PM, Sean Silva <silvas at purdue.edu> wrote:
> >
> > On Wed, Nov 13, 2013 at 1:59 PM, Dmitri Gribenko <gribozavr at gmail.com>
> > wrote:
> >> The original reason to provide these APIs was to implement a testing
> >> mode for comments in c-index-test.  These APIs were not meant for
> >> other uses, but I see that I failed to communicate that in the
> >> documentation at all.
> >
> > Why did you even put them in the header *and the exports list* if they
> > aren't public API?
>
> As I said, not communicating this is my fault.  I just added these
> APIs the same way as we usually add APIs to libclang.
>
> >> > Can you provide a concrete reason for wanting to remove these
> >> > APIs?  What actual change is it blocking?
> >>
> >> I think this is a separate issue, but here are some details.
> >>
> >> I'll be sending an RFC for this change as well, because I think that
> >> this is a major parsing *model* change.  It does not affect the
> >> resulting output in normal cases, but will do what the user intended
> >> in more unusual cases.
> >>
> >> On the AST side, it boils down to changing BlockCommandComment to
> >> contain multiple paragraphs inside, which will allow commands like
> >> \param to have multi-paragraph descriptions.  On libclang side, this
> >> change, for example, makes clang_BlockCommandComment_getParagraph()
> >> return incomplete results -- just the first paragraph.
> >
> > It seems like all they can do is get the text of the paragraph, so why
> not
> > just fake up a dummy paragraph whose text content is the content of all
> the
> > paragraphs? and document this restriction and point the user to a new API
> > that gives the full access to the paragraphs? I'm also really worried
> that
> > this will also change the XML schema in an incompatible way: the XML
> schema
> > is by extension part of the stability promise of the C API, and can't be
> > changed incompatibly without breaking users.
>
> Or we could only return the first paragraph.  Faking a paragraph
> requires creating AST nodes, which requires memory allocation on the
> ASTContext, which is permanent.
>

Why not create it on demand in "freeable" storage?


>
> The schema change is:
>
> Index: bindings/xml/comment-xml-schema.rng
> ===================================================================
> --- bindings/xml/comment-xml-schema.rng»(revision 194522)
> +++ bindings/xml/comment-xml-schema.rng»(working copy)
> @@ -393,7 +393,9 @@
>            <!-- In general, template parameters with whitespace discussion
>                 should not be emitted.  Schema might be more strict here.
> -->
>            <element name="Discussion">
> -            <ref name="TextBlockContent" />
> +            <oneOrMore>
> +              <ref name="TextBlockContent" />
> +            </oneOrMore>
>            </element>
>          </element>
>        </oneOrMore>
> @@ -435,7 +437,9 @@
>                 should not be emitted, unless direction is explicitly
> specified.
>                 Schema might be more strict here. -->
>            <element name="Discussion">
> -            <ref name="TextBlockContent" />
> +            <oneOrMore>
> +              <ref name="TextBlockContent" />
> +            </oneOrMore>
>            </element>
>          </element>
>        </oneOrMore>
>
> I think that this change is compatible (but it depends on how one
> defines "compatible").  Previously, <Discussion> tag could contain
> only a single paragraph; after this change it can contain multiple.
>

I'm not an XML expert, but that looks like it should be compatible for most
non-pathological use cases.

-- Sean Silva


>
> 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>*/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131113/b214ece8/attachment.html>