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

Douglas Gregor dgregor at apple.com
Thu Dec 20 09:30:35 PST 2012


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?

	- Doug


> 
> 
> +      else if ((Tok.Tok.is(tok::minus) || 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/16727272/attachment.html>


More information about the cfe-commits mailing list