================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
