[DWARFYAML] Use override instead of virtual for better safety.

[Higuoxing]

Summary

Functions in DWARFYML::FixupVisitor are declared as virtual functions in its base class DWARFYAML::Visitor. We should use the mordern "override" keyword instead of "virtual" for virtual functions in subclasses for better safety.

Besides, the visibility is changed from private to protected to make it consistent with DWARFYAML::FixupVisitor class and DWARFYAML::Visitor class.

[grimar]

Changing virtual -> override looks fine to me, but why did you change private -> protected?

[grimar]

Also, I find the description a bit confusing:

Virtual functions should be overridden in the derived class

Isn't this change (virtual->override) here a no-op to improve the code style?
All these funtions are anyways overriden, with the override word or without it.

[jhenderson]

Some points:

1. The virtual on the old version didn't declare a new virtual class. In face, it was actually superfluous from a strict point of view. virtual is propagated to all sub-class versions of functions with the same signature, regardless of accessor type. Prior to C++11, it was traditionally used to show that a sub-class function was an implementation of a virtual function in the point.
2. C++11 introduced the override specifier. It doesn't make a function any more or less virtual than it would have been before. All it does is require that the function overrides a virtual function in the parent class.
3. There's no need to change from private to protected. This just affects how functions can be called from the outside world - it doesn't affect the virtual nature of functions.

That all being said, there's nothing wrong with this change, since you're working in the area, in my opinion. Using override is always a good idea since it provides safety guarantees (rather than potentially accidentally creating a new virtual function). You just need to update your description and summary accordingly (something like "use override instead of virtual for better safety").

[jhenderson]

In D83452#2141125, @grimar wrote:

Also, I find the description a bit confusing:

Virtual functions should be overridden in the derived class

Isn't this change (virtual->override) here a no-op to improve the code style?
All these funtions are anyways overriden, with the override word or without it.

(I'm working on the assumption that @Higuoxing misunderstood the meaning of virtual and override - see my comment that landed at the same time as yours)

[tschuett]

See the c++ core guide rules:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override

[Higuoxing]

In D83452#2141125, @grimar wrote:

Also, I find the description a bit confusing:

Virtual functions should be overridden in the derived class

Isn't this change (virtual->override) here a no-op to improve the code style?
All these funtions are anyways overriden, with the override word or without it.

Yes, I know these functions are overridden with or without the override key word. Because the DIEFixupVisitor class is derived from DWARFYAML::Vistor, and those onXXXX() functions are declared as virtual functions in DWARFYAML::Vistor class. I want to make it stricter since if we use override keyword to override an non-existing virtual function, compiler will complain about it. For example, if we change the signature of functions in derived class by mistake, the compiler will spot it for us.

As for the change from private: to protected:, that's because these functions in DWARFYAML::Vistor and DWARFYAML::DumpVistor are declared in protected keyword, I want to make them consistent. Sorry for my poor expressions :( Does it make sense now?

In D83452#2141126, @jhenderson wrote:

Some points:

1. The virtual on the old version didn't declare a new virtual class. In face, it was actually superfluous from a strict point of view. virtual is propagated to all sub-class versions of functions with the same signature, regardless of accessor type. Prior to C++11, it was traditionally used to show that a sub-class function was an implementation of a virtual function in the point.
2. C++11 introduced the override specifier. It doesn't make a function any more or less virtual than it would have been before. All it does is require that the function overrides a virtual function in the parent class.

Yes! That's exactly what I want to do! See my comments for @grimar.

3. There's no need to change from private to protected. This just affects how functions can be called from the outside world - it doesn't affect the virtual nature of functions.

I want to make the coding style consistent since these functions in DWARFYAML::DIEFixupVisitor and DWARFYAML::Visitor are declared in protected keyword.

That all being said, there's nothing wrong with this change, since you're working in the area, in my opinion. Using override is always a good idea since it provides safety guarantees (rather than potentially accidentally creating a new virtual function). You just need to update your description and summary accordingly (something like "use override instead of virtual for better safety").

[jhenderson]

Okay, please update the summary and description to clarify this, because it looks like a number of people were confused!

[Higuoxing]

In D83452#2141178, @jhenderson wrote:

Okay, please update the summary and description to clarify this, because it looks like a number of people were confused!

Sorry for the inconvenience. Does it look better now?

[jhenderson]

We should override those functions rather than declare a new set of virtual functions for better safety.

We're still not declaring a new set of virtual functions. Just say something like "We should use the modern override keyword instead of virtual for virtual functions in subclasses."

[Higuoxing]

In D83452#2141201, @jhenderson wrote:

We should override those functions rather than declare a new set of virtual functions for better safety.

We're still not declaring a new set of virtual functions. Just say something like "We should use the modern override keyword instead of virtual for virtual functions in subclasses."

Thanks a lot! I've changed the description.

[jhenderson]

LGTM.

llvm/lib/ObjectYAML/DWARFEmitter.cpp

 
 namespace {
 class DIEFixupVisitor : public DWARFYAML::Visitor {
   uint64_t Length;
 
 public:
   DIEFixupVisitor(DWARFYAML::Data &DI) : DWARFYAML::Visitor(DI){};
 
-private:
-  virtual void onStartCompileUnit(DWARFYAML::Unit &CU) {
+protected:
+  void onStartCompileUnit(DWARFYAML::Unit &CU) override {
     // Size of the unit header, excluding the length field itself.
     Length = CU.Version >= 5 ? 8 : 7;
   }
 
-  virtual void onEndCompileUnit(DWARFYAML::Unit &CU) { CU.Length = Length; }
+  void onEndCompileUnit(DWARFYAML::Unit &CU) override { CU.Length = Length; }
 
-  virtual void onStartDIE(DWARFYAML::Unit &CU, DWARFYAML::Entry &DIE) {
+  void onStartDIE(DWARFYAML::Unit &CU, DWARFYAML::Entry &DIE) override {
     Length += getULEB128Size(DIE.AbbrCode);
   }
 
-  virtual void onValue(const uint8_t U) { Length += 1; }
-  virtual void onValue(const uint16_t U) { Length += 2; }
-  virtual void onValue(const uint32_t U) { Length += 4; }
-  virtual void onValue(const uint64_t U, const bool LEB = false) {
+  void onValue(const uint8_t U) override { Length += 1; }
+  void onValue(const uint16_t U) override { Length += 2; }
+  void onValue(const uint32_t U) override { Length += 4; }
+  void onValue(const uint64_t U, const bool LEB = false) override {
     if (LEB)
       Length += getULEB128Size(U);
     else
       Length += 8;
   }
-  virtual void onValue(const int64_t S, const bool LEB = false) {
+  void onValue(const int64_t S, const bool LEB = false) override {
     if (LEB)
       Length += getSLEB128Size(S);
     else
       Length += 8;
   }
-  virtual void onValue(const StringRef String) { Length += String.size() + 1; }
+  void onValue(const StringRef String) override { Length += String.size() + 1; }
 
-  virtual void onValue(const MemoryBufferRef MBR) {
+  void onValue(const MemoryBufferRef MBR) override {
     Length += MBR.getBufferSize();
   }
 };
 } // namespace
 
 Expected<StringMap<std::unique_ptr<MemoryBuffer>>>
 DWARFYAML::emitDebugSections(StringRef YAMLString, bool ApplyFixups,
                              bool IsLittleEndian) {