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

[Dmitri Gribenko]

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?

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>*/