[cfe-dev] Warning when calling virtual functions from constructor/desctructor?
Arnaud Bienner via cfe-dev
cfe-dev at lists.llvm.org
Tue Jan 8 05:37:39 PST 2019
Thanks for the detailed answer.
> The code pattern is definitely not always harmful. Most importantly, the
> behavior is only confusing if the method is actually overridden in a
subclass.
In this regard, note that my patch won't warn if the class is final.
Otherwise, indeed it might work as expected *now* but won't once
reimplemented in a subclass. Qualifying the call make things clearer.
> The best workaround is to name the current class, but programmers might
find
> that weird if it's not where the method is actually defined. If they
instead
> try to name the defining class, the workaround is making the code more
brittle.
Yes indeed: I named the *current* class instead of the defining one
intentionally in the fixit, because that would make the code behaves like
if it wasn't qualify, if one day the method gets reimplemented in the
current class.
If the developer decides qualify the call using the defining class anyway,
at least it will be clear what implementation will get called (but agreed
this would make the code more brittle).
I realized I didn't put "DefaultIgnore" on this warning.
I'm not experienced enough with clang to know what's the best way to deal
with new warnings, but my feeling is that it would be sensible to have this
new warning DefaultIgnore for now, in -Wall, and make it default once we
have some feedback from the community: while not all C++ projects use -Wall
(or -Wextra) I believe enough do to give us a chance to get some feedback.
What do you think?
Arnaud
Le mar. 8 janv. 2019 à 11:59, John McCall via cfe-dev <
cfe-dev at lists.llvm.org> a écrit :
> On 19 Dec 2018, at 9:22, Aaron Ballman via cfe-dev wrote:
>
> On Tue, Dec 18, 2018 at 6:59 PM Artem Dergachev via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>
> Static Analyzer's checker already has a mode in which it warns on case B,
> but it's off by default because it is surprisingly noisy. In particular, i
> vaguely recall that i've seen ~90 warnings on the OpenOffice codebase -
> none of them were pure virtual calls and most of them looked like false
> positives in the sense that the virtual function is called intentionally.
> Which means that i would be generally worried about this warning. That
> said, i don't know how warnings are accepted into Clang and whether there
> is a threshold on intentional violations. I suspect that it depends on
> whether the warning is expected to be on by default or be part of -Wall or
> only -Weverything.
>
> In general, warnings in Clang are expected to be on-by-default and
> have an extremely low false positive rate (certainly lower than for
> the static analyzer). I suspect this warning would not meet the usual
> bar for inclusion as a Clang warning given the number of false
> positives it's shown in the past. I think the static analyzer is a
> better place for the functionality to live, especially given that it
> requires actual static analysis of runtime control flows to reduce
> those false positives.
>
> Another good thing to do with a warning that may be intentionally
> triggered by the user is to add a hint on how to suppress it. Eg., how
> -Wparentheses does it:
>
> test.c:2:9: note: place parentheses around the assignment to silence this
> warning
> if (x = 1) {
> ^
> ( )
>
> I guess you could add a note that suggests to qualify the call with a
> class name, which is a good thing to do anyway. With that, in my opinion
> it'd be a good warning.
>
> Agreed that this would be a sensible way for users to suppress the
> diagnostic -- it clarifies that the programmer understands the
> behavior of the call.
>
> Agreed.
>
> In general, whether or not to add a new default-on warning comes down to
> two
> somewhat orthogonal considerations:
>
> -
>
> Are we willing to say that the code pattern is Wrong?
>
> This involves weighing the harm of unknowing uses of the code pattern
> against the harm of avoiding the diagnostic.
> -
>
> Are knowing, correct uses of the code pattern too common in practice
> to justify annoying a large fraction of the community?
>
> This involves weighing the diagnostic's impact on all code, which is
> impossible, so we have to muddle through with incomplete testing.
>
> For this particular diagnostic, I'm not sure about either of those points:
>
> -
>
> The code pattern is definitely not always harmful. Most importantly,
> the
> behavior is only confusing if the method is actually overridden in a
> subclass.
> -
>
> There's probably a lot of code which does this innocuously. The usual
> admonition that running code is usually correct is stronger than usual
> here
> because most code in a constructor or destructor will actually be run
> on the
> common path.
> -
>
> Some programmers will reasonably object that they know the semantics
> here.
> -
>
> The workaround can be a bit verbose.
> -
>
> The best workaround is to name the current class, but programmers
> might find
> that weird if it's not where the method is actually defined. If they
> instead
> try to name the defining class, the workaround is making the code more
> brittle.
>
> So I'm not sure if this is a good warning to add. I agree that it may be
> better
> suited for the static analyzer, which can try to ameliorate at least some
> of
> these concerns.
>
> *That said*, I'm concerned about the growing pressure to not add new
> warnings. That's not an intended goal of the pressure, but it is the basic
> outcome. There is a tightly-woven web of different factors that have the
> effect of discouraging the addition of any new diagnostics:
>
> -
>
> We discourage adding new default-off warnings on the theories that
> (1) warnings should have maximal impact and (2) we don't want unused
> complexity in the compiler.
> -
>
> New default-on warnings are immediately exercised on a massive amount
> of
> code and will be reverted if they cause any problems for existing code
> because
> we try to keep the codebase as close as possible to an acceptable
> release
> state at all times.
> -
>
> Project maintainers quite reasonably feel that (1) they did not ask
> for this
> new warning and (2) they don't really want to deal with it right now
> but (3)
> it's being forced on them anyway and (4) they are rather upset about
> that.
>
> The basic problem here is that we have no way for projects to evolve with
> default-on warnings, and so we necessarily conflate two very different
> ideas:
>
> -
>
> We want the warning to eventually be useful to as many people as
> possible.
> -
>
> We want the warning to be useful to everyone right now.
>
> But the first doesn't require the second. It would make a lot more sense
> if we curated an ever-expanding but *versioned* set of default-on warnings
> and allowed users to ratchet their version forward at their own pace.
> More precisely:
>
> 1.
>
> We would add a flag to control the current default diagnostic settings,
> something like -Wversion=clang-7 to say to use Clang 7's default
> warning
> behavior.
> 2.
>
> The first version of that would turn on a whole bunch of warnings that
> we've
> always thought ought to be on by default but aren't because of GCC
> compatibility.
> 3.
>
> In the absence of an explicit -Wversion flag, the compiler would
> continue
> using the supposedly-GCC-compatible defaults.
> 4.
>
> It would be understood that -Wversion=X+1 will generally be defined as
> -Wversion=X -Wfoo -Wbar -Wbaz. (Not literally in terms of the
> set-algebra
> of warnings flags, just in terms of each version adding new warnings.)
> 5.
>
> Project maintainers can roll forward their per-compiler warning
> versions
> when they're ready to deal with the consequences.
> 6.
>
> The compiler would not complain if it's passed a future version; it
> would just use its most recent rules. This allows projects to easily
> roll their warnings version forward while still working on older
> compilers.
> We would clearly document that projects which intentionally post-date
> their warnings version do so at their own risk.
> 7.
>
> Platform maintainers and distributors like Google and Apple (and other
> motivated projects like e.g. Mozilla) would periodically qualify the
> current
> -Wversion on their codebases in order to gather feedback on the current
> set of newly-added warnings.
> 8.
>
> Tools like Xcode can make their own decisions about rolling forward
> warning
> versions. For example, a new project might be configured to use the
> latest
> warning settings but then stay with that version until manually
> updated.
>
> This flag isn't meant to be a guarantee that we'll emit exactly the same
> diagnostics as the target version. For one, of course, we might change
> diagnostic text or emit the diagnostic somewhere else or add a note or
> whatever.
> More importantly, I think we'd still reserve the right to add new warnings
> that are unconditionally on by default if we think they're important or
> unproblematic enough.
>
> This wouldn't be a blank check to add new default-on warnings: the two
> basic
> considerations above would still apply. It's just that the second
> consideration would be far more focused:
>
> -
>
> It wouldn't need to account for the impact on legacy projects that
> might
> lack an active maintainer. Someone who wants to audit the new warnings
> on
> that project can easily do so, but otherwise the build is unaffected.
> -
>
> It wouldn't need to account for the inter-project politics of compilers
> forcing unwanted new warnings on other programmers. A project
> maintainer
> who intentionally rolls their version forward but doesn't like one of
> the
> new warnings can easily deal with that problem themselves.
> -
>
> Instead, it would just evaluate the impact from the perspective of a
> project
> maintainer who has voluntarily requested a larger set of warnings.
> Again,
> that wouldn't be a blank check: the warning still needs to be
> worthwhile.
> But it's necessarily a less fraught prospect.
>
> There would also be a very clear path for warnings under active
> development:
> our expectation would be that most new warnings would eventually be added
> to
> the set of default-on warnings. In my opinion, that leaves room for
> warnings
> to remain default-off as long as we expect that they will eventually become
> acceptable to include.
>
> John.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190108/13b9ea33/attachment.html>
More information about the cfe-dev
mailing list