[cfe-commits] clang formatter patch to move and rename RewriterTestContext.h
jahanian
fjahanian at apple.com
Tue Dec 18 15:06:43 PST 2012
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>*/
More information about the cfe-commits
mailing list