[cfe-commits] clang formatter patch to move and rename RewriterTestContext.h

[Daniel Jasper]

On Thu, Dec 20, 2012 at 6:30 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Dec 20, 2012, at 9:02 AM, Daniel Jasper <djasper at google.com> wrote:
>
> Hi,
>
> ok, some comments here, but after the discussions it seems like this can
> mostly go in as is.
>
> +    TT_BlockComment,
> +    TT_ObjectiveCMethodDecl
>    };
>
> The +/- actually determine whether it is an instance or a static method,
> right? If so, we might want to hint at that in this name.
>
>
> Yes, that's true. The formatter shouldn't care whether it's dealing with
> an instance vs. class method (they're conventionally formatted the same
> way), so why distinguish?
>

That's not what I meant. I meant something like
TT_ObjCStaticOrInstanceSpecifier (probably not a good name). But the enum
value should say some thing about the single token and I was suspecting
that the +/- by themselves are not method declarations.


> - Doug
>
>
>
>
> +      else if ((Tok.Tok.is <http://tok.tok.is/>(tok::minus) || Tok.Tok.is<http://tok.tok.is/>(tok::plus))
> &&
> +               Tok.Tok.isAtStartOfLine())
> +        Annotation.Type = TokenAnnotation::TT_ObjectiveCMethodDecl;
>
> I think it might be easier to set this in the AnnotatingParser. But that
> entire part needs cleanup so I'd be ok with you checking it in for now.
>
> Cheers,
> Daniel
>
>
> On Wed, Dec 19, 2012 at 12:06 AM, jahanian <fjahanian at apple.com> wrote:
>
>>
>> On Dec 17, 2012, at 9:44 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>
>> > On Mon, Dec 17, 2012 at 7:13 PM, jahanian <fjahanian at apple.com> wrote:
>> >> Are you going to provide the new public header in clang/Tooling this
>> week? I
>> >> have gone ahead and made the necessary changes for formatting ObjC
>> >> declarations and
>> >> and provide the tests in the attached patch which enforces our
>> convention
>> >> for formatting them.  Here is my complete patch (including the
>> plumbing for
>> >> declaration tags which you may ignore).  How do you want to proceed?
>> >
>> > +    // FIXME. For now focus on formatting objective-C declarations.
>> > +    if (isa<ObjCMethodDecl>(DI->CurrentDecl) ||
>> > +        isa<ObjCContainerDecl>(DI->CurrentDecl) ||
>> > +        isa<ObjCPropertyDecl>(DI->CurrentDecl) ||
>> > +        isa<ObjCIvarDecl>(DI->CurrentDecl) ||
>> > +        isa<ObjCPropertyImplDecl>(DI->CurrentDecl)) {
>> > +      // FIXME. formatting API expects null terminated input string.
>> > +      // There might be more efficient way of doing this.
>> > +      std::string StringDecl = Declaration.str();
>> > +      StringDecl += ";";
>> >
>> > Hello Fariborz,
>> >
>> > +      // Formatter specific code.
>> > +      // Form a unique in memory buffer name.
>> > +      llvm::SmallString<128> filename;
>> > +      filename += "xmldecl";
>> > +      filename += llvm::utostr(FormatInMemoryUniqueId);
>> > +      filename += ".xd";
>> > +      FileID ID = TContext.createInMemoryFile(filename, StringDecl);
>> > +      SourceLocation Start =
>> > +        TContext.Sources.getLocForStartOfFile(ID).getLocWithOffset(0);
>> > +      unsigned Length = Declaration.size();
>> > +
>> > +      std::vector<CharSourceRange> Ranges(1,
>> > +
>>  CharSourceRange::getCharRange(Start,
>> > +
>>  Start.getLocWithOffset(Length)));
>> > +      ASTContext &Context = DI->CurrentDecl->getASTContext();
>> > +      const LangOptions &LangOpts = Context.getLangOpts();
>> > +      Lexer Lex(ID, TContext.Sources.getBuffer(ID), TContext.Sources,
>> > LangOpts);
>> > +      tooling::Replacements Replace =
>> > +      reformat(format::getLLVMStyle(), Lex, TContext.Sources, Ranges);
>> > +      applyAllReplacements(Replace, TContext.Rewrite);
>> > +      appendToResultWithXMLEscaping(TContext.getRewrittenText(ID),
>> > true /*SkipSemi*/);
>> >
>> > Please move the whole format-decl-and-return-a-string dance to a
>> > separate static helper function:
>> > (1) it is logically separate,
>> > (2) CommentASTToXMLConverter::visitFullComment function is already
>> > very long and complicated.
>> >
>> > +      std::vector<CharSourceRange> Ranges(1,
>> > +
>>  CharSourceRange::getCharRange(Start,
>> > +
>>  Start.getLocWithOffset(Length)));
>> >
>> > Indentation is funny here.  If "CharSourceRange::" does not fit into
>> > 80 cols to be lined up with "1," then move "1," to a separate line.
>> >
>> > +      tooling::Replacements Replace =
>> > +      reformat(format::getLLVMStyle(), Lex, TContext.Sources, Ranges);
>> >
>> > Indentation.
>> >
>> > +      std::string StringDecl = Declaration.str();
>> > +      StringDecl += ";";
>> >
>> > Adding this semicolon should be the job of the pretty-printer.  For
>> > example, "namespace Foo {}" does not need a semicolon.  (Yes, I know
>> > it in objc-only now, but it is the declprinter that needs to be fixed,
>> > not this.)
>> >
>> > -void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef
>> S) {
>> > +void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef
>> S,
>> > +                                                             bool
>> SkipSemi) {
>> >   for (StringRef::iterator I = S.begin(), E = S.end(); I != E; ++I) {
>> >     const char C = *I;
>> >     switch (C) {
>> > @@ -1307,6 +1353,9 @@
>> >       case '\'':
>> >         Result << "'";
>> >         break;
>> > +      case ';':
>> > +        if (SkipSemi)
>> > +          break;
>> >
>> > As per above, adding a hack and then reversing is not a good approach,
>> > thus "SkipSemi" does not belong here.
>> >
>> > +  RewriterTestContext &TContext;
>> >
>> > A better variable name would be "FormatRewriterContext".
>> >
>> > +void *cxcursor::createFormatContext() {
>> > +  return new RewriterTestContext();
>> > +}
>> >
>> > +void cxcursor::disposeFormatContext(void *formatContext) {
>> > +  delete static_cast<RewriterTestContext*>(formatContext);
>> > +}
>> >
>> > Do we need these functions?  Why can not its only user
>> >
>> > --- tools/libclang/CIndex.cpp (revision 170208)
>> > +++ tools/libclang/CIndex.cpp (working copy)
>> > @@ -61,6 +61,8 @@
>> >   D->StringPool = createCXStringPool();
>> >   D->Diagnostics = 0;
>> >   D->OverridenCursorsPool = createOverridenCXCursorsPool();
>> > +  D->FormatContext = createFormatContext();
>> >
>> > just call new/delete directly?
>>
>> In r170467
>> - fariborz
>>
>> >
>> > 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
>>
>
> _______________________________________________
> 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/20121220/cafd768f/attachment.html>