Re: [lmi] Enabling '-Weffc++' by default

[Vadim Zeitlin]

On Tue, 9 Jun 2020 22:34:34 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> I had always thought that building with '-Weffc++' was sure to produce
GC> reams of output, mostly pointing to issues in third-party libraries
GC> beyond my control. However, apparently thanks again to '-isystem', it
GC> seems almost within our grasp for lmi, and wouldn't that be glorious?

 I'm very much sceptical about this. I believe this option is generally not
recommended for use and, moreover, never was meant to be used for anything
other than pedagogical purposes. It's also frozen in time and doesn't get
updated as the language evolves and I even believe that I saw somewhere
that bugs (i.e. false positives) in it will not be fixed neither.

 Let me quote the same gcc maintainer (it's just a coincidence, honestly)
again, this time from https://stackoverflow.com/a/11529328/15275

        GCC's -Weffc++ has several issues, I never use it. 

And it's just one of many similar quotes from different respected members
of C++ community I remember seeing about it.

 I think it would be far more productive to run clang static analyzer on
lmi instead of relying on a very poor (both in depth and breadth) and 20
years out of date approximation of what it does provided by this option.

GC> Specifically, if I do this in 'workhorse.make':
GC> 
GC>  gcc_common_extra_warnings := \
GC>    -Wcast-qual \
GC> +  -Weffc++ \
GC> 
GC> and then
GC>   make clobber && make install
GC> I still get reams of output, but almost all of it just says:
GC>   'foo' should be initialized in the member initialization list
GC> and it seems reasonable to suppose that all 5157 of them can and
GC> should be fixed. Then we'll have only the four '-Weffc++' issues
GC> listed below:
GC> 
GC> /opt/lmi/src/lmi/dbdict.hpp:37:14: error: base class ‘class 
cache_file_reads<DBDictionary>’ has accessible non-virtual destructor 
[-Werror=non-virtual-dtor]
GC> 
GC> /opt/lmi/src/lmi/html.hpp:137:7: error: ‘class html::attribute’ has pointer 
data members [-Werror=effc++]
GC> /opt/lmi/src/lmi/html.hpp:137:7: error:   but does not override 
‘html::attribute(const html::attribute&)’ [-Werror=effc++]
GC> /opt/lmi/src/lmi/html.hpp:137:7: error:   or ‘operator=(const 
html::attribute&)’ [-Werror=effc++]
GC> 
GC> /opt/lmi/src/lmi/html.hpp:166:14: error: ‘class html::detail::any_element’ 
has pointer data members [-Werror=effc++]
GC> /opt/lmi/src/lmi/html.hpp:166:14: error:   but does not override 
‘html::detail::any_element(const html::detail::any_element&)’ [-Werror=effc++]
GC> /opt/lmi/src/lmi/html.hpp:166:14: error:   or ‘operator=(const 
html::detail::any_element&)’ [-Werror=effc++]
GC> 
GC> /opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error: ‘class 
{anonymous}::standard_page’ has pointer data members [-Werror=effc++]
GC> /opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error:   but does not override 
‘{anonymous}::standard_page(const {anonymous}::standard_page&)’ [-Werror=effc++]
GC> /opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error:   or ‘operator=(const 
{anonymous}::standard_page&)’ [-Werror=effc++]
GC> 
GC> The first one (dbdict.hpp:37) is weird: it appears only with
GC> '-Weffc++', but is flagged "[-Werror=non-virtual-dtor]", so
GC> perhaps '-Weffc++' strengthens '-Wnon-virtual-dtor' checks?

 Yes, the documentation of this option says:

        This option also enables -Wnon-virtual-dtor, which is also one of
        the effective C++ recommendations. However, the check is extended
        to warn about the lack of virtual destructor in accessible
        non-polymorphic bases classes too. 

 But it still looks like a but to me, to be honest.

GC> Anyway, the class consists of just one typedef and one static
GC> member function, so AFAICS is shouldn't even have a (nonempty)
GC> vtable, and the warning is overly aggressive. But adding
GC> +    virtual ~cache_file_reads() = default;
GC> shuts it up, and seems harmless.

 I don't think it's harmless. Having this dtor gives the impression that
cache_file_reads<> is supposed to be used polymorphically, which results
in cognitive dissonance when you realize that it doesn't make any sense to
use it polymorphically. I agree with making the code slightly uglier to
avoid triggering otherwise useful warnings, but I don't believe that it's
worth doing it for the warnings which are just not very useful. If we
really want to enable -Weffc++ I'd rather use gcc pragmas to disable
-Wnon-virtual-dtor here.

GC> The last one (pdf_command_wx.cpp:1321) seems easy to fix by
GC> doing away with the pointer data member:

 Sure, but this warning is so bogus it hurts. It doesn't matter at all
that there is a pointer data member because it's a non-owning pointer!

GC> -    char const* const                    page_template_name_;
GC> +    std::string const                    page_template_name_;
GC>      std::unique_ptr<wxHtmlContainerCell> page_body_cell_;
GC>      std::unique_ptr<wxHtmlContainerCell> header_cell_;
GC> and I think that's correct. Vadim, was there some special
GC> reason for using a C string in this particular case?

 Simplicity. We always use a literal string here, there is no reason to
make a copy of it when we can just store a pointer.

GC> And would you please share your thoughts on the other two,
GC> (html.hpp:137) and (html.hpp:166)? I hesitate to replace
GC> "char const*" with string here, because I think you preferred
GC> C strings for efficiency

 Yes. Of course, we probably could afford making heap allocations/copies
here too, but the question is why? There is no real problem with this code
and, more generally, there doesn't seem any actual problem at all among
5157+4 problems found by enabling this option. 0/5161 doesn't seem to be a
good usefulness ratio to me and I'd rather forego this option completely.

 I think -Weffc++ provides an illusion of glory rather than anything more
concrete and I'd prefer not to spend time struggling against it, not only
now, but, inevitably, in the future, if it gets enabled.

 Regards,
VZ

pgpJMhtiMKFIN.pgp
Description: PGP signature