[PATCH] Branch handling cleanup in Consumed analysis

[Christian Wailes]

================
Comment at: lib/Analysis/Consumed.cpp:46
@@ -43,1 +45,3 @@
 
+#define INVERT_CONSUMED_UNCONSUMED(State) static_cast<ConsumedState>(State ^ 1)
+
----------------
David Blaikie wrote:
> I guess it was to allow you to write this?
> 
> This should just be a function (we don't much like macros) & I don't think it benefits all that much from the ability to bit twiddle it into inversion - I'd just write it out in a (file-local static) free function with conditional statements or conditional operators.
I would prefer the bit twiddling.  It should compile to a single XOR instruction, whereas a conditional would result in a branch, jump, and two move instructions if it is inlined (correct me if I'm wrong here).  If the bit twiddling is well documented, and wrapped in a macro or function, then it should be just as clear what is going on as if we had used a branch.

================
Comment at: include/clang/Analysis/Analyses/Consumed.h:95
@@ -92,3 +94,3 @@
     // No state information for the given variable.
-    CS_None,
+    CS_None = 0,
     
----------------
David Blaikie wrote:
> any particular reason to number these?
As you said, to allow the bit twiddling bellow.  Also to allow CS_None to represent false.

================
Comment at: lib/Analysis/Consumed.cpp:93
@@ -73,4 +92,3 @@
   
-  union PropagationUnion {
-    ConsumedState State;
-    const VarDecl *Var;
+  enum ConstantInfo {
+    CV_None,
----------------
David Blaikie wrote:
> unclear what this type represents - maybe renaming the values and/or adding comments would help
Unfortunately this is an artifact of me trying to clear my patch backlog.  These are to represent statically known information about tests and Boolean combinations of tests.  The enum is removed in the next patch.  I could combine the two, but then the patch would get rather large.

================
Comment at: lib/Analysis/Consumed.cpp:117
@@ +116,3 @@
+      IT_Var
+    } InfoType;
+    
----------------
David Blaikie wrote:
> should this really be an unencapsulated, modifiable public member?
Next patch set makes it private.

================
Comment at: lib/Analysis/Consumed.cpp:135
@@ -96,2 +134,3 @@
     
-    ConsumedState getState() const { return StateOrVar.State; }
+    const ConsumedState & getState() const { return InfoUnion.State; }
+    const RangeInfo     & getRange() const { return InfoUnion.Range; }
----------------
David Blaikie wrote:
> Are these intended to change the discriminated union state to the chosen one, or is there meant to be an invariant that you only call getState when isState, getRange when isRange, etc?
These are simple accessors into the discriminated union.  An assert is added in the next patch set to ensure the PropagationInfo object is in the correct state before returning the union value.

================
Comment at: lib/Analysis/Consumed.cpp:189
@@ +188,3 @@
+                            Entry->second.getRange().End);
+    } else {
+      return std::make_pair(0, 0);
----------------
David Blaikie wrote:
> "else after return" (drop the else, just have the second return be unconditional)
> and braces on single-statement conditionals - you can drop the braces (this isn't hard-and-fast in this case, some people allow/prefer/accept braces on multiline, single-statement blocks in LLVM)
Removed in the next patch.  Sorry that that is the response to so many of these.


http://llvm-reviews.chandlerc.com/D1480