<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 20, 2012, at 9:02 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">Hi,<div><br></div><div style="">ok, some comments here, but after the discussions it seems like this can mostly go in as is.</div><div style="">
<br></div><div style=""><div>+ TT_BlockComment,</div><div>+ TT_ObjectiveCMethodDecl</div><div> };</div><div><br></div><div style="">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.</div></div></div></div></blockquote><div><br></div><div>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?</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br></div><br><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div style="">
<div style=""><br></div><div style=""><br></div><div style=""><div>+ else if ((<a href="http://tok.tok.is/">Tok.Tok.is</a>(tok::minus) || <a href="http://tok.tok.is/">Tok.Tok.is</a>(tok::plus)) &&</div><div>+ Tok.Tok.isAtStartOfLine())</div>
<div>+ Annotation.Type = TokenAnnotation::TT_ObjectiveCMethodDecl;</div><div><br></div><div style="">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.</div>
</div></div><div style=""><br></div><div style="">Cheers,<br>Daniel</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 19, 2012 at 12:06 AM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On Dec 17, 2012, at 9:44 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>> wrote:<br>
<br>
> On Mon, Dec 17, 2012 at 7:13 PM, jahanian <<a href="mailto:fjahanian@apple.com">fjahanian@apple.com</a>> wrote:<br>
>> Are you going to provide the new public header in clang/Tooling this week? I<br>
>> have gone ahead and made the necessary changes for formatting ObjC<br>
>> declarations and<br>
>> and provide the tests in the attached patch which enforces our convention<br>
>> for formatting them. Here is my complete patch (including the plumbing for<br>
>> declaration tags which you may ignore). How do you want to proceed?<br>
><br>
> + // FIXME. For now focus on formatting objective-C declarations.<br>
> + if (isa<ObjCMethodDecl>(DI->CurrentDecl) ||<br>
> + isa<ObjCContainerDecl>(DI->CurrentDecl) ||<br>
> + isa<ObjCPropertyDecl>(DI->CurrentDecl) ||<br>
> + isa<ObjCIvarDecl>(DI->CurrentDecl) ||<br>
> + isa<ObjCPropertyImplDecl>(DI->CurrentDecl)) {<br>
> + // FIXME. formatting API expects null terminated input string.<br>
> + // There might be more efficient way of doing this.<br>
> + std::string StringDecl = Declaration.str();<br>
> + StringDecl += ";";<br>
><br>
> Hello Fariborz,<br>
><br>
> + // Formatter specific code.<br>
> + // Form a unique in memory buffer name.<br>
> + llvm::SmallString<128> filename;<br>
> + filename += "xmldecl";<br>
> + filename += llvm::utostr(FormatInMemoryUniqueId);<br>
> + filename += ".xd";<br>
> + FileID ID = TContext.createInMemoryFile(filename, StringDecl);<br>
> + SourceLocation Start =<br>
> + TContext.Sources.getLocForStartOfFile(ID).getLocWithOffset(0);<br>
> + unsigned Length = Declaration.size();<br>
> +<br>
> + std::vector<CharSourceRange> Ranges(1,<br>
> + CharSourceRange::getCharRange(Start,<br>
> + Start.getLocWithOffset(Length)));<br>
> + ASTContext &Context = DI->CurrentDecl->getASTContext();<br>
> + const LangOptions &LangOpts = Context.getLangOpts();<br>
> + Lexer Lex(ID, TContext.Sources.getBuffer(ID), TContext.Sources,<br>
> LangOpts);<br>
> + tooling::Replacements Replace =<br>
> + reformat(format::getLLVMStyle(), Lex, TContext.Sources, Ranges);<br>
> + applyAllReplacements(Replace, TContext.Rewrite);<br>
> + appendToResultWithXMLEscaping(TContext.getRewrittenText(ID),<br>
> true /*SkipSemi*/);<br>
><br>
> Please move the whole format-decl-and-return-a-string dance to a<br>
> separate static helper function:<br>
> (1) it is logically separate,<br>
> (2) CommentASTToXMLConverter::visitFullComment function is already<br>
> very long and complicated.<br>
><br>
> + std::vector<CharSourceRange> Ranges(1,<br>
> + CharSourceRange::getCharRange(Start,<br>
> + Start.getLocWithOffset(Length)));<br>
><br>
> Indentation is funny here. If "CharSourceRange::" does not fit into<br>
> 80 cols to be lined up with "1," then move "1," to a separate line.<br>
><br>
> + tooling::Replacements Replace =<br>
> + reformat(format::getLLVMStyle(), Lex, TContext.Sources, Ranges);<br>
><br>
> Indentation.<br>
><br>
> + std::string StringDecl = Declaration.str();<br>
> + StringDecl += ";";<br>
><br>
> Adding this semicolon should be the job of the pretty-printer. For<br>
> example, "namespace Foo {}" does not need a semicolon. (Yes, I know<br>
> it in objc-only now, but it is the declprinter that needs to be fixed,<br>
> not this.)<br>
><br>
> -void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef S) {<br>
> +void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef S,<br>
> + bool SkipSemi) {<br>
> for (StringRef::iterator I = S.begin(), E = S.end(); I != E; ++I) {<br>
> const char C = *I;<br>
> switch (C) {<br>
> @@ -1307,6 +1353,9 @@<br>
> case '\'':<br>
> Result << "'";<br>
> break;<br>
> + case ';':<br>
> + if (SkipSemi)<br>
> + break;<br>
><br>
> As per above, adding a hack and then reversing is not a good approach,<br>
> thus "SkipSemi" does not belong here.<br>
><br>
> + RewriterTestContext &TContext;<br>
><br>
> A better variable name would be "FormatRewriterContext".<br>
><br>
> +void *cxcursor::createFormatContext() {<br>
> + return new RewriterTestContext();<br>
> +}<br>
><br>
> +void cxcursor::disposeFormatContext(void *formatContext) {<br>
> + delete static_cast<RewriterTestContext*>(formatContext);<br>
> +}<br>
><br>
> Do we need these functions? Why can not its only user<br>
><br>
> --- tools/libclang/CIndex.cpp (revision 170208)<br>
> +++ tools/libclang/CIndex.cpp (working copy)<br>
> @@ -61,6 +61,8 @@<br>
> D->StringPool = createCXStringPool();<br>
> D->Diagnostics = 0;<br>
> D->OverridenCursorsPool = createOverridenCXCursorsPool();<br>
> + D->FormatContext = createFormatContext();<br>
><br>
> just call new/delete directly?<br>
<br>
</div></div>In r170467<br>
- fariborz<br>
<div class="im HOEnZb"><br>
><br>
> Dmitri<br>
><br>
> --<br>
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br>
<br>
</div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></blockquote></div><br></body></html>