[PATCH] D32009: Allow attributes with global variables

[Javed Absar via Phabricator via llvm-commits]

javed.absar added inline comments.


================
Comment at: include/llvm/IR/Attributes.h:568
+  /// The function attributes are returned.
+  AttributeList getAttributes() const { return Impl; }
+
----------------
jroelofs wrote:
> Usage of this encourages a lot of: `getAttributes().getAttributes()` which seems odd. @jmolloy/@javed.absar did you consider making this an `operator->()`?
> 
> Either way, shouldn't this return by reference, and not value (avoiding the copy)?
The AttributeList is thin (containing pointer to impl), so should not be a problem passing around by value.  Other usages e.g. class Function handles AttributeList  similarly by passing it around by value.



================
Comment at: include/llvm/IR/GlobalVariable.h:208
+  void setAttributes(AttributeListType A) { Attrs = A; }
+
   // Methods for support type inquiry through isa, cast, and dyn_cast:
----------------
tobiasvk wrote:
> hasAttribute() functions would be useful here (I see you already have them in SingleSlotAttributeList).
Yes, sorry missed this. Will add them soon and send new patch


================
Comment at: include/llvm/IR/GlobalVariable.h:204
+  /// Return the attribute list for this global
+  AttributeListType getAttributes() const { return Attrs; }
+
----------------
jroelofs wrote:
> Should this be return by reference instead of value (avoiding the copy)?
Same point as above.


https://reviews.llvm.org/D32009