[PATCH] D116110: Introduce the AttributeMask class

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/IR/Attributes.h:956
+    assert(Attribute::isEnumAttrKind(Val) &&
+           "Adding integer/type attribute without an argument!");
+    Attrs[Val] = true;
----------------
This assert doesn't make sense to me. AttributeMask doesn't accept arguments.


================
Comment at: llvm/include/llvm/IR/Attributes.h:993
+
+  bool empty() const { return Attrs.none(); }
+  bool hasAttributes() const { return TargetDepAttrs.empty() && Attrs.none(); }
----------------
Should this check TargetDepAttrs as well?


================
Comment at: llvm/include/llvm/IR/Attributes.h:994
+  bool empty() const { return Attrs.none(); }
+  bool hasAttributes() const { return TargetDepAttrs.empty() && Attrs.none(); }
+};
----------------
This looks inverted. Isn't this the implementation of empty()?


================
Comment at: llvm/include/llvm/IR/Attributes.h:1048
 
+  /// Remove the target-dependent attribute to the builder.
+  AttrBuilder &removeAttribute(Attribute A) {
----------------
to -> from


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

https://reviews.llvm.org/D116110