[PATCH] SymbolRewriter: introduce the SymbolRewriter pass

[Jim Grosbach]

There are a lot of definitions that appear to be private to the rewriter in the include/llvm/.../.h file. I suspect most of that content can move to an anonymous namespace in the .cpp file.

In general, the code needs a lot more comments. It's generally clean code, which is great for understand how things work, but comments are still useful for the "why" of things.

A few inline comments as well. Once some of this is sorted out, I may have additional feedback, but this will give us a good starting place, I hope.

================
Comment at: include/llvm/CodeGen/Passes.h:600
@@ -596,1 +599,3 @@
+  ModulePass *createRewriteSymbolsPass();
+  ModulePass *createRewriteSymbolsPass(SymbolRewriter::RewriteDescriptorList &);
 } // End llvm namespace
----------------
Nothing calls the second version.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:1
@@ +1,2 @@
+
+#ifndef LLVM_TRANSFORMS_UTILS_SYMBOL_REWRITER_H
----------------
Needs file level comment header. Should include a fairly substantive description of what the pass does.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:18
@@ +17,3 @@
+
+namespace SymbolRewriter {
+class RewriteDescriptor : public ilist_node<RewriteDescriptor> {
----------------
Why is there a SymbolRewriter namespace? The classes themselves should already provide the necessary scoping.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:26
@@ +25,3 @@
+public:
+  enum RewriteDescriptorType {
+    RDT_Invalid,
----------------
Comments on what these meen would be very helpful.

Why the RDT_ prefix? They're already namespaced inside the class.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:52
@@ +51,3 @@
+  ExplicitRewriteFunctionDescriptor(const StringRef S, const StringRef T,
+                                    const bool Naked)
+    : RewriteDescriptor(RDT_Function),
----------------
const on a non-reference argument?

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:63
@@ +62,3 @@
+
+class PatternRewriteFunctionDescriptor : public RewriteDescriptor {
+public:
----------------
In general, these classes all need some comments explaining how they fit together.

================
Comment at: lib/Transforms/Makefile:12
@@ -12,1 +11,3 @@
+PARALLEL_DIRS = Utils Instrumentation Scalar InstCombine IPO Vectorize Hello \
+		ObjCARC SymbolRewriter
 
----------------
Why is this in its own subdirectory? The header is in Utils (which seems to make sense), so the implementation should be, too, I'd think.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:474
@@ +473,3 @@
+
+  if (!(Transform.empty() ^ Target.empty())) {
+    YS.printError(Descriptor, "exactly one of transform or target must be specified");
----------------
Eh? Bitwise operator on the return values of empty()? That's a non-standard idiom. Rewriting in terms of boolean operators would make more sense.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:495
@@ +494,3 @@
+  RewriteSymbols();
+  RewriteSymbols(llvm::SymbolRewriter::RewriteDescriptorList &DL);
+
----------------
We have a "using namespace llvm" at the top. No need for the explicit scope operator here.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:544
@@ +543,3 @@
+llvm::ModulePass *
+llvm::createRewriteSymbolsPass(SymbolRewriter::RewriteDescriptorList &DL) {
+  return new RewriteSymbols(DL);
----------------
Nothing calls this version of the factory function?

http://reviews.llvm.org/D3161