Closed Bug 780618 Opened 12 years ago Closed 12 years ago

Move all error codes to nsError.h

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 777292 requires moving all error codes to nsError.h, but this can be done before actually making nsresult an enum.  We can do this before the other blockers are fixed.  I'll attach a patch when I have one working, should be soon.
Attached patch Patch (obsolete) — Splinter Review
Note that this removes the nsresult cast from NS_ERROR_GENERATE.  We don't want people generating new errors without using explicit casts.  Also, note that c-c will break with this change unless it replaces headers -- I believe

  s/#include "nsNetError.h"/#include "nsError.h"/

will be enough to fix it.
Attachment #649288 - Flags: review?(ehsan)
Attached patch Patch to fix comm-central (obsolete) — Splinter Review
This should make c-c compile again.  Obviously, this can't be pushed until the m-c patch earlier in the bug is.
Attachment #649605 - Flags: review?(mconley)
(In reply to :Aryeh Gregor from comment #1)
> Created attachment 649288 [details] [diff] [review]
> Patch
> 
> Note that this removes the nsresult cast from NS_ERROR_GENERATE.  We don't
> want people generating new errors without using explicit casts.

I don't necessarily agree that this is the right thing to do.  We definitely want comm-central to be able to use NS_ERROR_GENERATE to create their custom error messages, and I would rather them not having to change anything (such as adding nsresult casts everywhere they use NS_ERROR_GENERATE.)  Can you compile c-c successfully with these patches?  (I think it should fail to build...)
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> I don't necessarily agree that this is the right thing to do.  We definitely
> want comm-central to be able to use NS_ERROR_GENERATE to create their custom
> error messages, and I would rather them not having to change anything (such
> as adding nsresult casts everywhere they use NS_ERROR_GENERATE.)

I think c-c's error codes should be moved to nsError.h too, and in fact I have patches to do this.  (Other than some cases where it makes up nsresults on the fly, which m-c does too.)  Notably, if you switch on an enum, gcc warns if any of the case statements are not in the enum's definition, even if you cast the value you're comparing to:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54140

So if we keep error codes defined all over the place, we'd have to disable that warning, or at least make it -Wno-error.  I think it's better to just centralize all the error codes both m-c and c-c use.  (It would by far not be the only c-c-specific code in m-c.)  It's much tidier in any case, IMO.

> Can you
> compile c-c successfully with these patches?  (I think it should fail to
> build...)

Well, it builds fine, because nsresult is still typedefed to PRUint32.  When nsresult is changed to an enum, c-c will need patches to fix all this, which I have written and am going to submit as soon as they pass try.  There are a bunch of preparatory patches like I've written for m-c, to be landed as soon as they're reviewed.  And then there's a pair of patches for c-c and m-c to move all error definitions to nsError.h.


Since it turns out that various callers do make up error codes on the fly, though, I think it would be reasonable to add the cast back in to NS_ERROR_GENERATE_FAILURE, with a comment saying not to use it except when making up error codes dynamically.
(In reply to :Aryeh Gregor from comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > I don't necessarily agree that this is the right thing to do.  We definitely
> > want comm-central to be able to use NS_ERROR_GENERATE to create their custom
> > error messages, and I would rather them not having to change anything (such
> > as adding nsresult casts everywhere they use NS_ERROR_GENERATE.)
> 
> I think c-c's error codes should be moved to nsError.h too, and in fact I
> have patches to do this.  (Other than some cases where it makes up nsresults
> on the fly, which m-c does too.)

I don't think so.  It is not reasonable to expect every project which uses m-c to put their error codes in nsError.h.  c-c is just one example, and even though we have control over c-c, I'd rather treat it as a downstream project which we do not control to make sure that we won't break anybody by making nsresult an enum.

> Notably, if you switch on an enum, gcc
> warns if any of the case statements are not in the enum's definition, even
> if you cast the value you're comparing to:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54140
> 
> So if we keep error codes defined all over the place, we'd have to disable
> that warning, or at least make it -Wno-error.  I think it's better to just
> centralize all the error codes both m-c and c-c use.  (It would by far not
> be the only c-c-specific code in m-c.)  It's much tidier in any case, IMO.

This is a bug in gcc, and we can definitely disable that warning for gcc (please file a bug on that.)

> > Can you
> > compile c-c successfully with these patches?  (I think it should fail to
> > build...)
> 
> Well, it builds fine, because nsresult is still typedefed to PRUint32.  When
> nsresult is changed to an enum, c-c will need patches to fix all this, which
> I have written and am going to submit as soon as they pass try.  There are a
> bunch of preparatory patches like I've written for m-c, to be landed as soon
> as they're reviewed.  And then there's a pair of patches for c-c and m-c to
> move all error definitions to nsError.h.

Right, I meant, would building c-c work with this patch and also the one which makes nsresult an enum?  It won't build, and we need to make sure that we keep it build just fine as an exercise to make sure that this conversion will not break anyone who uses the Gecko code base.

> Since it turns out that various callers do make up error codes on the fly,
> though, I think it would be reasonable to add the cast back in to
> NS_ERROR_GENERATE_FAILURE, with a comment saying not to use it except when
> making up error codes dynamically.

I think you should add the cast back, without that warning.  Fact of the matter is that people have used their own custom nsresult values and there is no good way to support this properly in C++ (because you cannot have partial enums).

However, if you want to improve things in m-c, you can define another set of macros which do not perform the casts, and use them in the m-c as needed.  For example:

#define NS_DEFINE_ERROR_CODE  ... // the no-cast version
#define NS_ERROR_GENERATE_FAILURE(foo) ((nsresult)NS_DEFINE_ERROR_CODE(foo))

// use NS_DEFINE_ERROR_CODE in Gecko
Comment on attachment 649288 [details] [diff] [review]
Patch

r- based on the above.
Attachment #649288 - Flags: review?(ehsan) → review-
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> I think you should add the cast back, without that warning.  Fact of the
> matter is that people have used their own custom nsresult values and there
> is no good way to support this properly in C++ (because you cannot have
> partial enums).

You might not consider this is a "good way", but one possibility is to include something like this:

enum nsresult {
 NS_OK,
...
#ifdef CUSTOM_NSRESULT_CODES_H
# include CUSTOM_NSRESULT_CODES_h
#endif
};
(In reply to comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > I think you should add the cast back, without that warning.  Fact of the
> > matter is that people have used their own custom nsresult values and there
> > is no good way to support this properly in C++ (because you cannot have
> > partial enums).
> 
> You might not consider this is a "good way", but one possibility is to include
> something like this:
> 
> enum nsresult {
>  NS_OK,
> ...
> #ifdef CUSTOM_NSRESULT_CODES_H
> # include CUSTOM_NSRESULT_CODES_h
> #endif
> };

My goal is to make sure that projects using m-c will not have to modify anything as a result of this conversion.
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> I don't think so.  It is not reasonable to expect every project which uses
> m-c to put their error codes in nsError.h.  c-c is just one example, and
> even though we have control over c-c, I'd rather treat it as a downstream
> project which we do not control to make sure that we won't break anybody by
> making nsresult an enum.

We're not breaking anyone by making nsresult an enum.  However, to take maximal advantage of enum features, we should move any error codes we can to nsError.h.  For one, it allows us to not disable a possibly-useful gcc warning.  It also means that debuggers will provide more useful info for variables.  If all error codes are in nsError.h, "p res" will show you the name of the variable, not just the numeric code, right?  There are most likely other benefits too.

So *breaking* other projects is one thing, but I don't think that means we shouldn't do what's best for our own projects, even if we can't do it for other projects.  Regardless of what we do for c-c, other projects will have to disable gcc's warnings and deal with less useful debugging info if they mint their own errors.  I don't see why c-c should have to do that too, let alone m-c.

> This is a bug in gcc, and we can definitely disable that warning for gcc
> (please file a bug on that.)

Why should we disable it when we can move all the error codes to nsError.h instead?  If third parties make their own errors and want to use them in switch statements, they can disable the warning themselves easily enough for their own compilation units.

> I think you should add the cast back, without that warning.  Fact of the
> matter is that people have used their own custom nsresult values and there
> is no good way to support this properly in C++ (because you cannot have
> partial enums).

So I'm okay with making this change.  This means that other users will break slightly less than they would otherwise.  In practice, adding the casts here by hand is a tiny percentage of the work required to get stuff to compile, so it doesn't save much.  But I still think we should move all the codes we can to nsError.h, because it makes more sense to centralize them.  This will not prevent others linking with Gecko from defining new codes (nothing we do will, in fact).

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> My goal is to make sure that projects using m-c will not have to modify
> anything as a result of this conversion.

That's not possible.  Any place they assign a numeric type to an nsresult in some fashion will break with this change.  From experience, I can tell you that's lots and lots of places.  The large bulk of changes needed will be precisely because of the type safety that we want here, and we can't avoid that.

Besides, what exactly are we concerned about here?  Do we really consistently try to support arbitrary third parties who link against our code?  That would mean we can't do any kind of refactoring that changes public interfaces.  Why is this worse than getting rid of nsnull?
(In reply to comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > I don't think so.  It is not reasonable to expect every project which uses
> > m-c to put their error codes in nsError.h.  c-c is just one example, and
> > even though we have control over c-c, I'd rather treat it as a downstream
> > project which we do not control to make sure that we won't break anybody by
> > making nsresult an enum.
> 
> We're not breaking anyone by making nsresult an enum.  However, to take maximal
> advantage of enum features, we should move any error codes we can to nsError.h.
>  For one, it allows us to not disable a possibly-useful gcc warning.  It also
> means that debuggers will provide more useful info for variables.  If all error
> codes are in nsError.h, "p res" will show you the name of the variable, not
> just the numeric code, right?  There are most likely other benefits too.
> 
> So *breaking* other projects is one thing, but I don't think that means we
> shouldn't do what's best for our own projects, even if we can't do it for other
> projects.  Regardless of what we do for c-c, other projects will have to
> disable gcc's warnings and deal with less useful debugging info if they mint
> their own errors.  I don't see why c-c should have to do that too, let alone
> m-c.

For sure.  What I'm concerned with is _assuming_ that all (or most) nsresult values will go inside the same enum.  Once nsresult is an enum, we can definitely file a c-c bug and let them decide if they want to move those values to nsresult, but I think we should be conservative and assume the answer is no.  Which is why the casts are needed.

> > This is a bug in gcc, and we can definitely disable that warning for gcc
> > (please file a bug on that.)
> 
> Why should we disable it when we can move all the error codes to nsError.h
> instead?  If third parties make their own errors and want to use them in switch
> statements, they can disable the warning themselves easily enough for their own
> compilation units.

Yeah, what I was trying to say is that a gcc bug which causes a warning to be emitted shouldn't hold much weight in our decision making process.

> > I think you should add the cast back, without that warning.  Fact of the
> > matter is that people have used their own custom nsresult values and there
> > is no good way to support this properly in C++ (because you cannot have
> > partial enums).
> 
> So I'm okay with making this change.  This means that other users will break
> slightly less than they would otherwise.  In practice, adding the casts here by
> hand is a tiny percentage of the work required to get stuff to compile, so it
> doesn't save much.

So, for example if you attempt to compile c-c with nsresult being an enum and with these casts in place, what other things need to be fixed?

> But I still think we should move all the codes we can to
> nsError.h, because it makes more sense to centralize them.  This will not
> prevent others linking with Gecko from defining new codes (nothing we do will,
> in fact).

I think we should move as much in m-c as possible.  We should just not assume that we can always do this.

> (In reply to Ehsan Akhgari [:ehsan] from comment #8)
> > My goal is to make sure that projects using m-c will not have to modify
> > anything as a result of this conversion.
> 
> That's not possible.  Any place they assign a numeric type to an nsresult in
> some fashion will break with this change.  From experience, I can tell you
> that's lots and lots of places.  The large bulk of changes needed will be
> precisely because of the type safety that we want here, and we can't avoid
> that.

Those types of breakages are expected and fine since they are abuses of nsresult.  But breakages because we decided to remove a cast will cause code that is arguably still valid to break.

> Besides, what exactly are we concerned about here?  Do we really consistently
> try to support arbitrary third parties who link against our code?  That would
> mean we can't do any kind of refactoring that changes public interfaces.  Why
> is this worse than getting rid of nsnull?

nsnull is not necessarily used in every binary extension.  nsresult probably is, because it is emitted from the XPIDL parser.
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> For sure.  What I'm concerned with is _assuming_ that all (or most) nsresult
> values will go inside the same enum.  Once nsresult is an enum, we can
> definitely file a c-c bug and let them decide if they want to move those
> values to nsresult, but I think we should be conservative and assume the
> answer is no.  Which is why the casts are needed.

Okay, fair enough.

> So, for example if you attempt to compile c-c with nsresult being an enum
> and with these casts in place, what other things need to be fixed?

See bug 779130, where I have a patchset to fix everything in c-c that breaks -- *not* counting moving the definitions of error codes to nsError.h.  Stat is:

 89 files changed, 543 insertions(+), 469 deletions(-)

If NS_ERROR_GENERATE_FAILURE did the cast itself, I believe it would be:

 88 files changed, 532 insertions(+), 464 deletions(-)

Note that one of those cases actually uses an nsresult as the input to NS_ERROR_GENERATE_FAILURE, which is completely bogus, so it would need to be fixed anyway if NS_ERROR_GENERATE_FAILURE were made type-safe and nsresult were an enum class.  (Both of which are good ideas.)


Anyway, no sense in arguing for the sake of arguing.  I think we agree that errors we control should be moved to nsError.h, at least from m-c, but NS_ERROR_GENERATE_FAILURE should keep working without extra casts.  So I'll write patches that do that.
(In reply to comment #11)
> > So, for example if you attempt to compile c-c with nsresult being an enum
> > and with these casts in place, what other things need to be fixed?
> 
> See bug 779130, where I have a patchset to fix everything in c-c that breaks --
> *not* counting moving the definitions of error codes to nsError.h.  Stat is:
> 
>  89 files changed, 543 insertions(+), 469 deletions(-)
> 
> If NS_ERROR_GENERATE_FAILURE did the cast itself, I believe it would be:
> 
>  88 files changed, 532 insertions(+), 464 deletions(-)

Ah crap.  I didn't know that the number of nsresult misuses is this high in c-c.  :(

> Anyway, no sense in arguing for the sake of arguing.  I think we agree that
> errors we control should be moved to nsError.h, at least from m-c, but
> NS_ERROR_GENERATE_FAILURE should keep working without extra casts.  So I'll
> write patches that do that.

Thanks!  :-)
Try: https://tbpl.mozilla.org/?tree=Try&rev=d14c07b9bf73

I don't have a proper interdiff for you, but if I diff the diffs, here are the only non-context lines that changed:

-+    (((PRUint32)(sev) << 31) | \
-+     ((PRUint32)(module + NS_ERROR_MODULE_BASE_OFFSET) << 16) | \
-+     ((PRUint32)(code)))
++    (nsresult)(((PRUint32)(sev) << 31) | \
++               ((PRUint32)(module + NS_ERROR_MODULE_BASE_OFFSET) << 16) | \
++               ((PRUint32)(code)))
Attachment #649288 - Attachment is obsolete: true
Attachment #651304 - Flags: review?(ehsan)
Attachment #651304 - Flags: review?(ehsan) → review+
Comment on attachment 649605 [details] [diff] [review]
Patch to fix comm-central

Review of attachment 649605 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me - but while you're here, can you please update the documentation that refers to nsNetError.h in these files:

/calendar/providers/wcap/calWcapErrors.js 
/calendar/resources/content/calendarService.js
/chat/modules/socket.jsm

Thanks,

-Mike
Attachment #649605 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1cb80516a00

(In reply to Mike Conley (:mconley) from comment #14)
> This looks fine to me - but while you're here, can you please update the
> documentation that refers to nsNetError.h in these files:
> 
> /calendar/providers/wcap/calWcapErrors.js 
> /calendar/resources/content/calendarService.js
> /chat/modules/socket.jsm

I'll attach a new patch that does that.  I won't push it now, because the m-c patch is only on m-i, and I don't want to break c-c until the next m-i merge.  I guess someone else can push the patch to c-c when it's needed, or maybe I'll do it after the next merge.
Anyone who's around should feel free to check this in when c-c breaks, assuming I don't get to it first.
Attachment #649605 - Attachment is obsolete: true
Attachment #651677 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a1cb80516a00
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 651677 [details] [diff] [review]
Patch v2 to fix comm-central, addresses review comments

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/ca6d6339aee9
You need to log in before you can comment on or make changes to this bug.