Which part of the unit tests do you think is bulky? The initial 
infrastructure is mostly a copy from StmtPrinterTest.cpp. This shouldn't need 
to be changed when adding tests (and probably should be moved to some common 
code), so the size of the actual tests is similar to what is needed for 
FileCheck.

  I used FileCheck for the tests for my standalone dumper, and had a couple of 
problems. It:
  * uses the same compiler options and language for the whole test
  * currently ignores leading whitespace so can't test indentation
  * is fragile if you want to test the SourceRange dumping

  The ability to use an AST matcher for unit tests also makes them slightly 
more convenient (I realise there'll be a command line option for this 
eventually).

  The main advantage I can see for FileCheck is that it provides more flexible 
matching.


================
Comment at: lib/AST/StmtDumper.cpp:47
@@ -70,1 +46,3 @@
+          while (CI)
+            DumpSubTree(*CI++);
         }
----------------
Alexander Kornienko wrote:
> How about:
>   for (Stmt::child_range CI = S->children(); CI; ++CI)
>     DumpSubTree(*CI);
> ?
Fixed.

================
Comment at: lib/AST/StmtDumper.cpp:52
@@ -75,10 +51,3 @@
       }
-      --IndentLevel;
-    }
-
-    void DumpDeclarator(Decl *D);
-
-    void Indent() const {
-      for (int i = 0, e = IndentLevel; i < e; ++i)
-        OS << "  ";
+      Dumper.unindent();
     }
----------------
Alexander Kornienko wrote:
> Did you consider replacing indent/unindent pairs with something like:
> ```class IndentScope {
>   ASTDumper &Dumper;
> public:
>   IndentScope(ASTDumper &Dumper) : Dumper(Dumper) {
>     Dumper.indent();
>   }
>   ~IndentScope() {
>     Dumper.unindent();
>   }
> };
> ...
> while (...) {
>   IndentScope Indent(Dumper);
>   ...
> }
> ```
> ?
I didn't, but I like that; done. I also added a flush() to ~ASTDumper().


http://llvm-reviews.chandlerc.com/D52
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to