814674 - Add FAIL_ON_WARNINGS to accessible/src/atk and accessible/build

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

What is it for?

Comment 3

[Jonathan Watt [:jwatt]]

Warnings can hide real issues, and having bogus issues being reported is a pain because it's hard to see if you introduced new warnings. As a result people just end up ignoring them and having noisy builds. The addition of FAIL_ON_WARNINGS and the push to add it to all our makefiles is to avoid these issues.

Comment 4

[alexander :surkov (:asurkov)]

So if I introduce new warning then build just fails or?

You fixed only two Makefiles because those folders don't have any warnings?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to alexander :surkov from comment #4)
> So if I introduce new warning then build just fails or?

In directories with FAIL_ON_WARNINGS, GCC & clang will treat compile-warnings as compile-errors. (but only in builds with "ac_add_options --enable-warnings-as-errors" in their mozconfig (including TBPL builds)).

So yes -- if you introduce a build warning in those directories, then the build will fail. (which is good, because that build warning you introduced could indicate a bug in your patch -- and even if it's not, it'd make it harder for other people to notice bugs in their patches, per comment 3, so it's good to fix.)

> You fixed only two Makefiles because those folders don't have any warnings?

(speaking on jwatt's behalf) almost certainly yes.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > So if I introduce new warning then build just fails or?
> 
> In directories with FAIL_ON_WARNINGS, GCC & clang will treat
> compile-warnings as compile-errors. (but only in builds with "ac_add_options
> --enable-warnings-as-errors" in their mozconfig (including TBPL builds)).

Will it be default option of debug build? In other words should I care to add this option into my local mozconfig?

> So yes -- if you introduce a build warning in those directories, then the
> build will fail. (which is good, because that build warning you introduced
> could indicate a bug in your patch -- and even if it's not, it'd make it
> harder for other people to notice bugs in their patches, per comment 3, so
> it's good to fix.)

I agree, warnings are often left unnoticed and it must help to this problem. However sometimes warnings are introduced when you take a certain approach and actually you don't have any idea how to get rid of them (for example, we have such warnings related to Relation class). Should warnings be a stopper for the approach? Or do we have a way to say we are aware of those warnings and we are fine keep them for a while?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to alexander :surkov from comment #6)
> (In reply to Daniel Holbert [:dholbert] from comment #5)
> > (In reply to alexander :surkov from comment #4)
> > > So if I introduce new warning then build just fails or?
> > 
> > In directories with FAIL_ON_WARNINGS, GCC & clang will treat
> > compile-warnings as compile-errors. (but only in builds with "ac_add_options
> > --enable-warnings-as-errors" in their mozconfig (including TBPL builds)).
> 
> Will it be default option of debug build? In other words should I care to
> add this option into my local mozconfig?

It's not default, because we don't want to break builds of people using obscure GCC / clang variants. (e.g. if a new contributor comes along who happens to be running GCC 4.Next with bonus warnings in ostensibly warning-free directories, we don't want to break their build.)

You should add it to your local mozconfig, yeah -- at least, I'd recommend doing so.

> I agree, warnings are often left unnoticed and it must help to this problem.
> However sometimes warnings are introduced when you take a certain approach
> and actually you don't have any idea how to get rid of them [...]
> Should warnings be a stopper

This is a more general question, and this bug probably isn't the best place to discuss it (and we've had discussions along these lines in various places already).

But basically, my understanding of the consensus/best-practice is:
   (a) If most instances of a particular warning are reasonably likely to be ignorable, then we can selectively remove it from FAIL_ON_WARNINGS' purview. (We did this for uninitialized-variable warnings in opt builds, since we have many spurious instances of that warning.)

...and otherwise:
   (b) If a warning happens to be ignorable in a particular instance, we can & should work around it (by e.g. adding a static_cast) to avoid spamming up everyone's build. And if necessary, I think we can disable the warning for that specific line or file.

...and generally:
   (c) while we may not expend too much effort fixing the warnings in every single area of existing code, we might as well mark already-warning-free code as such, to avoid regressing it.

Comment 8

[alexander :surkov (:asurkov)]

Ok, thank you, Daniel.

Comment 9

[Trevor Saunders (:tbsaunde)]

> 
> I agree, warnings are often left unnoticed and it must help to this problem.

I think its fairly unfair to say unnoticed.  I'd tend to say that we prioritize a number of things higher than fixing warnings which I believe is completely reasonable.

> However sometimes warnings are introduced when you take a certain approach
> and actually you don't have any idea how to get rid of them (for example, we
> have such warnings related to Relation class). Should warnings be a stopper

actually I think that warning is  msvc only, gcc 4.7 doesn't warn about it atleast.  I believe that warning is fixable too if somebody has / wants to spend the time reading and thinking about the relavent bits of the spec.

>    (b) If a warning happens to be ignorable in a particular instance, we can
> & should work around it (by e.g. adding a static_cast) to avoid spamming up
> everyone's build. And if necessary, I think we can disable the warning for
> that specific line or file.

I dont really like this because atleast in the case of signed comparison warnings I think your usually just hiding the bug.  Also if someday the api does get fixed and the warning goes away people may well forget to remove all the casts that aren't needed anymore.

>    (c) while we may not expend too much effort fixing the warnings in every
> single area of existing code, we might as well mark already-warning-free
> code as such, to avoid regressing it.

That is if we believe that the priority of keeping things warning free is higher than the priority of doing whatever you were doing in the first place, which I think often isn't the case.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> >    (b) If a warning happens to be ignorable in a particular instance, we can
> > & should work around it (by e.g. adding a static_cast) to avoid spamming up
> > everyone's build. And if necessary, I think we can disable the warning for
> > that specific line or file.
> 
> I dont really like this because atleast in the case of signed comparison
> warnings I think your usually just hiding the bug.

I think you misunderstand me. I agree that simply adding a cast could potentially hide the bug, and I don't advocate blindly adding casts.  I only ever add casts when I know it's safe, e.g. here for the myInt < myUnsignedInt comparison:

  int myInt = SomeFuncThatReturnsASignedInt();
  if (myInt >= 0 &&
      myInt < myUnsignedInt) {
  }

> Also if someday the api
> does get fixed and the warning goes away people may well forget to remove
> all the casts that aren't needed anymore.

I agree, we shouldn't blindly add casts to existing code just to silence warnings. I'd generally only advocate adding casts-to-silence-warnings in cases like the one above, where we know the cast is valid and where it makes that assumption explicit.

> >    (c) while we may not expend too much effort fixing the warnings in every
> > single area of existing code, we might as well mark already-warning-free
> > code as such, to avoid regressing it.
> 
> That is if we believe that the priority of keeping things warning free is
> higher than the priority of doing whatever you were doing in the first
> place, which I think often isn't the case.

I don't think of it as "the priority of keeping things warning-free (for its own sake)".

I rather think of it as "the priority of _noticing_ new build warnings, which could very well indicate bugs in your new code and which at the very least will increase spam in the builds of everyone else." IMHO *that* priority (of noticing new build warnings) is reasonably high.  That said, if you notice the build warnings and decide they're not worth fixing and it's actually blocking work, you can always remove the FAIL_ON_WARNINGS annotation, as a last resort.

So far, the vast majority of FAIL_ON_WARNINGS annotations have stuck & they've caught real bugs (mostly minor, e.g. incorrect signed/unsigned comparisons & unused variables, but likely some important ones as well), so I think on balance, it's a good to use it when we can.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > >    (b) If a warning happens to be ignorable in a particular instance, we can
> > > & should work around it (by e.g. adding a static_cast) to avoid spamming up
> > > everyone's build. And if necessary, I think we can disable the warning for
> > > that specific line or file.
> > 
> > I dont really like this because atleast in the case of signed comparison
> > warnings I think your usually just hiding the bug.
> 
> I think you misunderstand me. I agree that simply adding a cast could
> potentially hide the bug, and I don't advocate blindly adding casts.  I only
> ever add casts when I know it's safe, e.g. here for the myInt <
> myUnsignedInt comparison:

ok, that's a bit better though I'm still sort of concerned about people adding casts as bustage fixes.

> > Also if someday the api
> > does get fixed and the warning goes away people may well forget to remove
> > all the casts that aren't needed anymore.
> 
> I agree, we shouldn't blindly add casts to existing code just to silence
> warnings. I'd generally only advocate adding casts-to-silence-warnings in
> cases like the one above, where we know the cast is valid and where it makes
> that assumption explicit.

and things shouldn't be changed someday so the cast isn't necessary?

For example we plenty of things like
int x = bar->GetFoo();
uint y = Bar();
return x < y;

one could certainly make that return x > 0 && static_cast<uint>(x) > y; but then when things did get fixed you'd have an extra branch.

> > >    (c) while we may not expend too much effort fixing the warnings in every
> > > single area of existing code, we might as well mark already-warning-free
> > > code as such, to avoid regressing it.
> > 
> > That is if we believe that the priority of keeping things warning free is
> > higher than the priority of doing whatever you were doing in the first
> > place, which I think often isn't the case.
> 
> I don't think of it as "the priority of keeping things warning-free (for its
> own sake)".
> 
> I rather think of it as "the priority of _noticing_ new build warnings,

assuming dice have a memory, and the warnings people might add in the future are similar to the ones we have now I'm fairly comfortable with people not noticing warnings absolutely immediately.

> which could very well indicate bugs in your new code and which at the very

a bug which is most likely not very important.

> least will increase spam in the builds of everyone else." IMHO *that*
> priority (of noticing new build warnings) is reasonably high.  That said, if

I don't really buy the spam is terrible argument, perhaps others work differently, but I really don't mind a few lines much at all, other than there's a bug there that needs to be worked on.

> So far, the vast majority of FAIL_ON_WARNINGS annotations have stuck &
> they've caught real bugs (mostly minor, e.g. incorrect signed/unsigned
> comparisons & unused variables, but likely some important ones as well), so
> I think on balance, it's a good to use it when we can.

if it isn't clear I'd tend to disagree, especially when most of the code can only be built on one platform.  FOr example accessible/src/atk/ is linux only.  Fighting with try to get stuff to build is bad enough as it is, I'm not very interested in things which make that take even more work, which I expect will be especially true here for people working on windows.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> and things shouldn't be changed someday so the cast isn't necessary?
>
> For example we plenty of things like
> int x = bar->GetFoo();
> uint y = Bar();
> return x < y;
> 
> one could certainly make that return x > 0 && static_cast<uint>(x) > y; but
> then when things did get fixed you'd have an extra branch.

Sure -- of course we'd want to fix that once GetFoo() is "fixed" to return an unsigned value.

But in the meantime, supposing that GetFoo() returns a (maybe-negative) int, it's *much* better to have the ">=0" and "static_cast" in there, to give us safer code with clearer assumptions. :)  Would you prefer we keep that example-code as-is & do the wrong thing whenever GetFoo() returns -1?

(Also: depending on how GetFoo() is hypothetically "fixed", the compiler may even help you find now-removable-branches, with build warnings telling you that "comparison of unsigned >= 0 is always true".  And if those happen to be in in FAIL_ON_WARNINGS directories, that'll make sure you notice those and fix them. :)  So there won't be a problem.)

> assuming [...] the warnings people might add in the future
> are similar to the ones we have now I'm fairly comfortable with people not
> noticing warnings absolutely immediately.

I don't think the warnings we have now are a good predictor of the warnings people might add in the future.

By definition, the warnings we see in our current code are exactly the ones that haven't seemed important enough (& haven't been about broken-enough stuff) to motivate us to fix them yet.  So it's no surprise that the warnings we have tend to seem unimportant.

Build warnings in *new* code, on the other hand, could be more serious & could indicate real bugs.  We may eventually catch & fix any that are serious (possibly at the cost of a security-bug bounty, in extreme cases), but we might as well notice & fix them sooner rather than later.

> > which could very well indicate bugs in your new code and which at the very
> 
> a bug which is most likely not very important.

Maybe important, maybe not (per above) -- but at the very least, it's a mistake -- it's up to a human to determine how important it is.

Also, as noted in comment 7, if we decide that a particular class of warnings are generally "most likely not very important", we can definitely change FAIL_ON_WARNINGS to ignore those. (and we've already done that, in at least one case so far)

> I don't really buy the spam is terrible argument, perhaps others work
> differently, but I really don't mind a few lines much at all

I don't mind a few lines here and there either..  But if we don't do something like FAIL_ON_WARNINGS, then a few lines will inevitably grow to many, in many areas of the code. And then people won't be able to notice build warnings (i.e. potential bugs) in their own build output, because there's so much background-noise.

> I'm
> not very interested in things which make that take even more work, which I
> expect will be especially true here for people working on windows.

I agree that this is where FAIL_ON_WARNINGS causes the most pain -- "my patch compiles fine on my machine, but it busted linux builds on TBPL and now I have to backout".  I feel people's pain there, but on balance, I still think it's worth it.  (And this can be mostly addressed with sanity-check Try pushes & the occasional backout when this happens).

I wish we had a better solution -- suggestions are welcome :)

Comment 13

[alexander :surkov (:asurkov)]

Trev, what about patch? Do you have concerns why we shouldn't take it?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > and things shouldn't be changed someday so the cast isn't necessary?
> >
> > For example we plenty of things like
> > int x = bar->GetFoo();
> > uint y = Bar();
> > return x < y;
> > 
> > one could certainly make that return x > 0 && static_cast<uint>(x) > y; but
> > then when things did get fixed you'd have an extra branch.
> 
> Sure -- of course we'd want to fix that once GetFoo() is "fixed" to return
> an unsigned value.
> 
> But in the meantime, supposing that GetFoo() returns a (maybe-negative) int,
> it's *much* better to have the ">=0" and "static_cast" in there, to give us
> safer code with clearer assumptions. :)  Would you prefer we keep that
> example-code as-is & do the wrong thing whenever GetFoo() returns -1?

I'd probably say it depends, I think I can come up with cases where adding checks certainly makes sense, and I think I could see cases where its better to leave things as they are as a nag to actually fix things.

> (Also: depending on how GetFoo() is hypothetically "fixed", the compiler may
> even help you find now-removable-branches, with build warnings telling you
> that "comparison of unsigned >= 0 is always true".  And if those happen to
> be in in FAIL_ON_WARNINGS directories, that'll make sure you notice those
> and fix them. :)  So there won't be a problem.)

on the other hand it increases the barier to fixing the return type because it'll take more work.

> > assuming [...] the warnings people might add in the future
> > are similar to the ones we have now I'm fairly comfortable with people not
> > noticing warnings absolutely immediately.
> 
> I don't think the warnings we have now are a good predictor of the warnings
> people might add in the future.
> 
> By definition, the warnings we see in our current code are exactly the ones
> that haven't seemed important enough (& haven't been about broken-enough
> stuff) to motivate us to fix them yet.  So it's no surprise that the
> warnings we have tend to seem unimportant.
> 
> Build warnings in *new* code, on the other hand, could be more serious &
> could indicate real bugs.  We may eventually catch & fix any that are
> serious (possibly at the cost of a security-bug bounty, in extreme cases),
> but we might as well notice & fix them sooner rather than later.

When I say existing warnings I should probably say the warnings existing now, and the ones fixed in the last year and a half.

I certainly admit its possible, but I think the warnings we would introduce in the nextyear will be fairly simmilar to the ones introduced over the last.

> > > which could very well indicate bugs in your new code and which at the very
> > 
> > a bug which is most likely not very important.
> 
> Maybe important, maybe not (per above) -- but at the very least, it's a
> mistake -- it's up to a human to determine how important it is.

it might well not be a mistake so much as a stupid compiler or a stupid language.

> > I don't really buy the spam is terrible argument, perhaps others work
> > differently, but I really don't mind a few lines much at all
> 
> I don't mind a few lines here and there either..  But if we don't do
> something like FAIL_ON_WARNINGS, then a few lines will inevitably grow to
> many, in many areas of the code. And then people won't be able to notice

I think the number of warnings I see in accessible/ today is the same or less than a year ago so I don't think its true we need FAIL_ON_WARNINGS to do this.

> build warnings (i.e. potential bugs) in their own build output, because
> there's so much background-noise.

saddly I think we live in a world where the problem is fixing known bugs, not looking for things that may be bugs.

> > I'm
> > not very interested in things which make that take even more work, which I
> > expect will be especially true here for people working on windows.
> 
> I agree that this is where FAIL_ON_WARNINGS causes the most pain -- "my
> patch compiles fine on my machine, but it busted linux builds on TBPL and
> now I have to backout".  I feel people's pain there, but on balance, I still
> think it's worth it.  (And this can be mostly addressed with sanity-check
> Try pushes & the occasional backout when this happens).

I'd disagree, I think needing to do more try pushes alone is enough of a pain the claimed benefits aren't worth it.

> I wish we had a better solution -- suggestions are welcome :)

honestly I don't think we have enough of a problem for it to be worth spending much time trying to solve.

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> Trev, what about patch? Do you have concerns why we shouldn't take it?

other than I think its a waste of time and generally a bad idea?

Comment 16

[Daniel Holbert [:dholbert]]

Just clarifying a few things, and then giving up on arguing here...

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > Maybe important, maybe not (per above) -- but at the very least, it's a
> > mistake -- it's up to a human to determine how important it is.
> 
> it might well not be a mistake so much as a stupid compiler or a stupid
> language.

As I mentioned several times already, any warnings that primarily fall into this category can be removed from FAIL_ON_WARNINGS' purview.

> I think the number of warnings I see in accessible/ today is the same or
> less than a year ago so I don't think its true we need FAIL_ON_WARNINGS to
> do this.

Maybe! And this annotation would let machines help us keep it at that (admirably low!) level. :)

> > build warnings (i.e. potential bugs) in their own build output, because
> > there's so much background-noise.
> 
> saddly I think we live in a world where the problem is fixing known bugs,
> not looking for things that may be bugs.

I wasn't talking about "looking for things that may be bugs".  That's not what this is for.

I was talking about catching bugs/warnings in *work-in-progress* code -- being able to catch warnings/bugs in your new code, **as you're writing it**.  If there's too much warning-spam, you won't notice build warnings in your own work-in-progress code.  Hopefully that clarifies that point.

> honestly I don't think we have enough of a problem for it to be worth
> spending much time trying to solve.

OK -- clearly you & I disagree on this. :)

As this is your area of code (and it's not even my patch -- I just have a stake in it, having written some of the FAIL_ON_WARNINGS-support), I'll stop arguing and accept your decision.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> (In reply to alexander :surkov from comment #13)
> > Trev, what about patch? Do you have concerns why we shouldn't take it?
> 
> other than I think its a waste of time and generally a bad idea?

For accessibility module only or in general? If in general then this bug is probably not a right place to discuss that.

Comment 18

[Takanori MATSUURA]

FAIL_ON_WARNING seems to be set to accessible/atk and accessible now.

Comment 19

[Daniel Holbert [:dholbert]]

(Annotation added in accessible/atk in bug 1110031. accessible/build no longer exists.)