Subzero: Emit ARM build-attributes in the file scope (as header).

src/IceTargetLoweringARM32.h

Index: src/IceTargetLoweringARM32.h
diff --git a/src/IceTargetLoweringARM32.h b/src/IceTargetLoweringARM32.h
index 53c98951993af7a4d08d0d09230877f74bf250fb..61a5df700a924731d76e3bce6d526f61db551360 100644
--- a/src/IceTargetLoweringARM32.h
+++ b/src/IceTargetLoweringARM32.h
@@ -324,6 +324,25 @@ private:
   template <typename T> static void emitConstantPool(GlobalContext *Ctx);
 };
 
+class TargetHeaderARM32 : public TargetHeaderLowering {

[John, 2015/06/05 22:28:30]

Maybe the class should be final, and then you don't have to label each virtual
method as such?

[jvoung (off chromium), 2015/06/08 22:21:10]

Is that the preferred style?

I could only find "Explicitly annotate overrides of virtual functions or virtual
destructors with an override or (less frequently) final specifier. Older
(pre-C++11) code will use the virtual keyword as an inferior alternative
annotation. For clarity, use exactly one of override, final, or virtual when
declaring an override. Rationale: A function or destructor marked override or
final that is not an override of a base class virtual function will not compile,
and this helps catch common errors. The specifiers serve as documentation; if no
specifier is present, the reader has to check all ancestors of the class in
question to determine if the function or destructor is virtual or not."

Previously we had been marking the methods individually as override or final.
Can marking the class final help find cases where you rename the method in one
part but forget to rename in others?

Otherwise, I could mark the class final if you all prefer that style.

[John, 2015/06/08 23:22:26]

There's very little difference in marking a class as final versus each
individual methods. Marking individual methods is useful for it allows more
"modular" final methods. However, marking a whole class as final shows that you
did not intend the class to be derived, which explicitly tells what the
"semantics" for the class are. Final, for methods, are usually used when you
want to "refine" the class behavior. For example:

class Inst {
 ...
 public:
  virtual string EmitAsm() = 0;
 ...
};

class InstIntelX86Return : public Inst {
 public:
  string EmitAsm() final {
    return "\tmov " + ReturnRegister() + ", " + Operand()\n"
           "\tret";
  }

  virtual string ReturnRegister() = 0;
};

class InstIntelX8632Return final : public InstIntelX86Return {
 public:
  string ReturnRegister() {
    return "eax";
  }
};

class InstIntelX8664Return final : public InstIntelX86Return {
 public:
  string ReturnRegister() {
    return "rax";
  }
};

[jvoung (off chromium), 2015/06/11 22:12:32]

Ah, my original worry was if you rename ReturnRegister to ReturnReg, but forget
to do that everywhere. I guess that should at least tell you that you've got an
unimplemented method:

warning: abstract class is marked 'final' [-Wabstract-final-class]
class InstIntelX8664Return final : public InstIntelX86Return {


Looks like Clang at least gives that above error, but GCC 4.8 won't actually
give you an error for the "class C final" version compared to the method final
version. Perhaps later GCC versions has gotten that right in a later version.

Okay, changed to class final.

+  TargetHeaderARM32() = delete;
+  TargetHeaderARM32(const TargetHeaderARM32 &) = delete;
+  TargetHeaderARM32 &operator=(const TargetHeaderARM32 &) = delete;
+
+public:
+  static TargetHeaderLowering *create(GlobalContext *Ctx) {
+    return new TargetHeaderARM32(Ctx);

[John, 2015/06/05 22:28:29]

why not returning a

std::unique_ptr<TargetHeaderLowering> here?

[Jim Stichnoth, 2015/06/08 20:23:43]

I also think that sounds reasonable, here and for x86-32.  The caller may need
to std::move the result, but that's OK.

[jvoung (off chromium), 2015/06/08 22:21:10]

Done.

+  }
+
+  void lower() final;
+
+protected:
+  explicit TargetHeaderARM32(GlobalContext *Ctx);
+
+private:
+  ~TargetHeaderARM32() final {}

[John, 2015/06/05 22:28:30]

= default; instead of {}?

[jvoung (off chromium), 2015/06/08 22:21:10]

Done.

+};
+
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICETARGETLOWERINGARM32_H