[PATCH] Refactoring SimplifyLibCalls to remove static initializers and generally cleaning up the code.

[Robin Morisset]

It mostly seems reasonable to me, but I don't know this area of the code, and do not feel at all comfortable giving an LGTM, so someone more experienced should look at it.

My only complaint about the patch as a whole is that it could probably have been cut in several parts, in particular all the whitespace/formatting changes could have been a separate patch and it would have made reviewing much easier.

Nitpicks inline.

================
Comment at: include/llvm/Transforms/Utils/SimplifyLibCalls.h:57
@@ +56,3 @@
+
+  bool hasFloatVersion(StringRef FuncName);
+
----------------
Could you give a comment here for the intended use/semantics for this method ?
Does it need to be public ?

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1626
@@ +1625,3 @@
+        CI->getArgOperand(0), CI->getArgOperand(1),
+        ConstantInt::get(DL->getIntPtrType(CI->getContext()), // Copy the
+                         FormatStr.size() + 1),
----------------
The comment here got mangled by the reformatting.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1902
@@ -2124,1 +1901,3 @@
   if (TLI->getLibFunc(FuncName, Func) && TLI->has(Func)) {
+    // We never change the calling convention.
+    if (!ignoreCallingConv(Func) && !isCallingConvC)
----------------
This looks like a behavior change. If so it should probably be in a separate patch (this one is already rather bulky).

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:2046
@@ -2265,1 +2045,3 @@
 
+  if (!isCallingConvC)
+    return nullptr;
----------------
Same as before, this does not look like just a part of the refactor.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:2071
@@ -2297,6 +2070,3 @@
                                      bool UnsafeFPShrink) {
-  Impl = new LibCallSimplifierImpl(DL, TLI, this, UnsafeFPShrink);
-}
-
-LibCallSimplifier::~LibCallSimplifier() {
-  delete Impl;
+  this->DL = DL;
+  this->TLI = TLI;
----------------
This could perhaps be replaced by an initializer list ?

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:2076
@@ -2304,6 +2075,3 @@
 
-Value *LibCallSimplifier::optimizeCall(CallInst *CI) {
-  if (CI->isNoBuiltin()) return nullptr;
-  return Impl->optimizeCall(CI);
-}
+LibCallSimplifier::~LibCallSimplifier() {}
 
----------------
Is this necessary ? If it does nothing I would be tempted to just keep the default destructor.

http://reviews.llvm.org/D5364