[PATCH] D21261: [CFLAA] Code cleanup: group all graph-building codes into one class

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch.


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:592
@@ +591,3 @@
+class CFLGraphBuilder {
+private:
+  // Input of the builder
----------------
Redundant `private`

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:732
@@ +731,3 @@
+
+  CFLGraph getCFLGraph() { return std::move(Graph); }
+  SmallVector<Value *, 4> getReturnValues() {
----------------
Things that look and feel like accessors (but that visibly modify the state of their object) seem sketchy to me.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:855
@@ -858,4 +854,3 @@
 CFLAAResult::FunctionInfo CFLAAResult::buildSetsFrom(Function *Fn) {
-  CFLGraph Graph;
-  SmallVector<Value *, 4> ReturnedValues;
 
+  CFLGraphBuilder GraphBuilder(*this, TLI, *Fn);
----------------
Useless newline

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:859
@@ -865,2 +858,3 @@
 
+  auto Graph = GraphBuilder.getCFLGraph();
   SmallVector<Value *, 16> Worklist;
----------------
I don't like that this builder is essentially partially-alive (`Graph` has been moved from, `ReturnedValues` hasn't) for the majority of this function.

Can we either:

- Have get*() return references (that we move from at the end of the function), or
- Move everything out of Builder at the top of the function, and declare it entirely dead+useless

instead?


http://reviews.llvm.org/D21261