Abstract class `CommandLineFlag` has non-virtual destructor

[uhle]

The destructor of the class CommandLineFlag is missing the keyword virtual although this class has lots of pure virtual methods defining an interface. While this is generally a bad idea, this also results in compiler warnings if compiling the webrtc-audio-processing library (version 1.0) for instance. The following is what GCC is complaining:

In file included from /usr/include/absl/flags/internal/flag.h:35,
                 from /usr/include/absl/flags/flag.h:39,
                 from ../webrtc/modules/audio_processing/agc2/rnn_vad/rnn_vad_tool.cc:15:
/usr/include/absl/flags/commandlineflag.h:62:7: warning: ‘class absl::debian1::CommandLineFlag’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
In file included from /usr/include/absl/flags/flag.h:39,
                 from ../webrtc/modules/audio_processing/agc2/rnn_vad/rnn_vad_tool.cc:15:
/usr/include/absl/flags/internal/flag.h:440:7: warning: base class ‘class absl::debian1::CommandLineFlag’ has accessible non-virtual destructor [-Wnon-virtual-dtor]

This bug can trivially be solved like that:

--- a/absl/flags/commandlineflag.h
+++ b/absl/flags/commandlineflag.h
@@ -153,7 +153,7 @@
   bool ParseFrom(absl::string_view value, std::string* error);
 
  protected:
-  ~CommandLineFlag() = default;
+  virtual ~CommandLineFlag() = default;
 
  private:
   friend class flags_internal::PrivateHandleAccessor;

[derekmauro]

Previous report: https://github.com/abseil/abseil-cpp/pull/928

Unfortunately the proposed change causes other problems, see the previous report.

[uhle]

Derek, thank you for the hint! I have searched for something like that but haven't found it.

However, I think that the explanation given at https://github.com/abseil/abseil-cpp/pull/928#issuecomment-813623379 is not sufficient. The only sentence I can accept is:

It is intentionally non-virtual to keep the base class "literal".

I do understand that because making the destructor virtual would add it to the vtable which would be necessary to call the correct destructor of a derived class at runtime if a pointer to a base class object is all the delete operator has got. Making the destructor of the base class protected does not provide anything similar. Why should defining a derived class as final have any influence on the base class? It would not stop me from deriving another class from this base class for instance.

I honestly hope that GCC is also providing such warnings in the future, just because I do not think that the compiler might be wrong in this or any other similar case. In contrast, you should be glad and take such a warning as a strong hint that you might rethink your implementation details. Maybe a wrapper class without virtual methods could be a solution for you letting you define literals while also having the implementation (now hidden behind the wrapper class) in a polymorphic fashion, or maybe even a template class approach if there are some methods with arguments of varying types that are not derived from a common base class for instance. There is almost always more than one way to implement something.

[Kevinlinpr]

@uhle But you should know that the protected destructor is never and ever facing the resource leak problem, for example you can provide the protected destructor when you want others only use the derived class, if you design the virtual member function in the base class and hope others to inheriting to create new derived class ,then you should make the destructor be virtual to avoid the resource leak problem when someone use the base class pointer to delete its derived class object or deleted automatically on stack.