[PATCH] D15524: [GCC] Attribute ifunc support in clang

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/AST/DeclBase.h:563
@@ +562,3 @@
+  /// \brief Return true if this declaration is a definition of alias or ifunc.
+  bool hasDefiningAttr() const;
+
----------------
I think this function and getDefiningAttr() can be defined in the header instead of split into the source file. The implementations are short enough that inlining may be nice to allow.

================
Comment at: include/clang/AST/DeclBase.h:565
@@ +564,3 @@
+
+  /// \brief Returns AliasAttr or IFuncAttr if any or nullptr.
+  Attr* getDefiningAttr() const;
----------------
rjmccall wrote:
> "Return this declaration's defining attribute if it has one."
> 
> Also, please put the * next to the method name rather than the type.
Also, since this is a const method, I'd prefer if it returned a `const Attr *` if possible.

================
Comment at: lib/AST/DeclBase.cpp:369
@@ +368,3 @@
+
+Attr* Decl::getDefiningAttr() const {
+  if (AliasAttr *AA = getAttr<AliasAttr>())
----------------
Attr* -> Attr *

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1538
@@ +1537,3 @@
+  // Aliases should be on declarations, not definitions.
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    if (FD->isThisDeclarationADefinition()) {
----------------
Can use cast<> instead of dyn_cast<> and remove the if.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1543
@@ +1542,3 @@
+    }
+    if (S.Context.getTargetInfo().getTriple().getObjectFormat() !=
+            llvm::Triple::ELF) {
----------------
I think that this should be codified in Attr.td as a target-specific attribute; though that may require a bit of work for the attribute emitter to handle. I don't think this needs to be done in this patch, but it should at least have a FIXME or be handled in a follow-up patch.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1549
@@ +1548,3 @@
+  } else
+    llvm_unreachable("ifunc must be used for function declaration");
+
----------------
Can remove the else clause entirely; common attribute handling already takes care of this and by switching to use cast<> instead of dyn_cast<> you already get an assert if the type is incorrect.


http://reviews.llvm.org/D15524