[cfe-dev] Warning when calling virtual functions from constructor/desctructor?

[Arthur O'Dwyer via cfe-dev]

On Thu, Dec 13, 2018 at 1:21 PM Arnaud Bienner via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> What I like about having this kind of diagnostics being part of clang
> itself is that you can turn them into errors using -Werror=, and prevent
> bugs to even been written, saving you debugging time.
> I'm not sure how static code analysis is done on the projects you are
> working on, but from my experience, it is usually done as part of the CI
> (so after you commit your changes): this saves you from having bugs
> reaching the release stage, but doesn't save you the time spent debugging a
> silly error that could have been catch by the compiler.
>

FWIW, I think most projects run CI on pull requests, so, the code has to
pass CI before it's allowed to hit master. (This is how you keep your
master CI green all the time, instead of flashing red-green-red-green.)

I think any diagnostic in this area is really too fragile to be useful.
Consider:

    struct A {
        A() { do_foo(); }  // diagnosable and definitely a bug
        virtual void do_foo() = 0;
    };
    struct B {
        B() { do_foo(); }  // diagnosable, but possibly correct
        virtual void do_foo();
    };
    struct C {
        C() { foo(); }  // definitely a bug, but not diagnosable
        void foo() { do_foo(); }  // best practice
    private:
        virtual void do_foo() = 0;
    };

Clang, GCC, and ICC all diagnose "A". Nobody diagnoses "B". Nobody
diagnoses "C", even though it's exactly isomorphic to "A" — and "C"
(splitting customization points into a public non-virtual interface and a
private virtual implementation) is what we want to teach people to do!

I think Clang can't *remove* its diagnostic for "A" because that would be a
regression versus GCC and ICC; but I don't think it's worth spending time
teaching Clang to catch "B" (with its attendant false positives) given that
"C" is the most interesting case in practice, and Clang will never be able
to catch "C".

The static analyzer, though, could definitely catch "C"! I don't know if it
does today or not.

my $.02,
–Arthur
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181213/688529fc/attachment.html>