[PATCH] D135572: [Attributes] Support int attributes with zero value

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: llvm/lib/IR/Attributes.cpp:102
+  else
+    assert(Val == 0 && "Value must be zero for enum attributes");
 
----------------
aeubanks wrote:
> seems dangerous to put `assert` in the else branch without parentheses, it might expand to nothing?
that's ok, the assert will never eat the semi column.


================
Comment at: llvm/lib/IR/Attributes.cpp:1663
+             getRawIntAttr(Attribute::VScaleRange).value_or(0))
+      .first;
 }
----------------
I would be tempted to change the signature of thatone and the above to return an Optional instead of defaulting to zero, what do you think?


================
Comment at: llvm/lib/IR/Attributes.cpp:1667
 Optional<unsigned> AttrBuilder::getVScaleRangeMax() const {
-  return unpackVScaleRangeArgs(getRawIntAttr(Attribute::VScaleRange)).second;
+  return unpackVScaleRangeArgs(
+             getRawIntAttr(Attribute::VScaleRange).value_or(0))
----------------
Based on the implementation of `unpackVScaleRangeArgs` I think you can early return `None` if the original Attr is None here


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135572/new/

https://reviews.llvm.org/D135572