1119652 - Modern clang fails the build because of missing override keyword on bindings generated class

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

4:22.07 ../../dist/include/mozilla/dom/ErrorEvent.h:38:23: error: 'AsErrorEvent' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
 4:22.07   virtual ErrorEvent* AsErrorEvent();
 4:22.07                       ^

This happens with ProgressEvent as well.  The As...Event() methods generated for these two classes override the base version but the rest of these methods do not, so we cannot tag these methods as MOZ_OVERRIDE in CGEventClass.

I can't really think of a good way to fix this unfortunately.  Should we resort to a Bindings.conf variable to say whether a method should be overload?  Can we do something less ugly?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the point is that Event only defines AsErrorEvent/AsProgressEvent but we output As*Event for all generated event classes, right?  That's moderately annoying.

Can we disable this particular warning for dom/bindings?  Would we even want to?

One stupid option would be to just hardcode in the code generator which event classes get As*Event.  Or yes, put it in bindings.conf or IDL...

Or add all possible As*Event to Event.h, but that's not all that nice either when someone adds a new event type.

I'm not seeing any pretty solutions here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The least ugly solution would be to disable the warning, if possible.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #1) 
> Or add all possible As*Event to Event.h, but that's not all that nice either
> when someone adds a new event type.

yeah, although for codegen'ed events we could create some .h file which Event.h then includes in the right place.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Creating .h and including it in Event.h would be probably the optimal solution.
(would we need to include it twice? One for forward declarations and once for As*Event implementations?)
I quickly looked at how to do that, but handling of webidl files in the build system is so convoluted these days that gave up pretty quickly.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This would be a global .h that is regenerated any time an event interface is added, right?

Seems like cribbing what we do for PrototypeList.h should work...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> This would be a global .h that is regenerated any time an event interface is
> added, right?
Yup

> Seems like cribbing what we do for PrototypeList.h should work...
ah

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

https://tbpl.mozilla.org/?tree=Try&rev=371bd47b2e6c

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8546867 [details] [diff] [review]
wip

Seems to compile.
(and I tried locally adding and removing codegen'ed events)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546867 [details] [diff] [review]
wip

>+        for generatedEvent in config.generatedEvents:

I think I'd modestly prefer:

  return CGList(CGGeneric(declare=("GENERATED_EVENT(%s)\n" % generatedEvent))
                for generatedEvent in config.generatedEvents)

but either way.

>+        self._generated_events_stems_as_array = generated_events_stems;

Is this guaranteed sorted?  If not, sort, please, so we have a consistent ordering?

r=me

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #9)
>   return CGList(CGGeneric(declare=("GENERATED_EVENT(%s)\n" % generatedEvent))
>                 for generatedEvent in config.generatedEvents)
Personally I really hate that syntax ;) Makes code so much harder to read.

> >+        self._generated_events_stems_as_array = generated_events_stems;
> 
> Is this guaranteed sorted?  If not, sort, please, so we have a consistent
> ordering?
So I decided to use the array, not items in the set just because of sorting.
the array here should be in the order they are in the moz.build

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea97247b91a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ea97247b91a