Bug 42250 – Code generator: ensure generated constants match their corresponding enums.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marcus Bulach]

Code generator: ensure generated constants match their corresponding enums.

[Marcus Bulach]

Created attachment 61507 [details]
Patch

[WebKit Review Bot]

Attachment 61507 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1243:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1248:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1253:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1258:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1263:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:178:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:179:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:180:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:181:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:182:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1025:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 11 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

As suggested in:
https://bugs.webkit.org/show_bug.cgi?id=41888

The code generator now generates compile-time checks for const / enums.
Notes:
1. There are ~32 exceptions (including 1 possible real mismatch), but real checks are generated for other ~35 classes.

2. This is done only for V8 at the moment. If you think this is something useful, I'll add to the other generators as well.

3. Most of this patch contains auto-generated (i.e., rebaselined/golden) files to test the bindings.

Please, let me know your thoughts on this.

[Jeremy Orlow]

Comment on attachment 61507 [details]
Patch

WebCore/bindings/scripts/CodeGeneratorV8.pm:1549
 +  sub shouldCheckEnum
We should add a new attribute to these classes in their IDLs rather than adding stuff to the code generator, I think.

I think I like that we're black listing (rather than white listing) though.  That way people adding constants in the future will need to opt out rather than in.

[Marcus Bulach]

Created attachment 61523 [details]
Patch

[WebKit Review Bot]

Attachment 61523 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1250:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1255:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1260:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1265:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1270:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:178:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:179:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:180:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:181:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:182:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1026:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 12 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

(In reply to comment #4)
> (From update of attachment 61507 [details])
> WebCore/bindings/scripts/CodeGeneratorV8.pm:1549
>  +  sub shouldCheckEnum
> We should add a new attribute to these classes in their IDLs rather than adding stuff to the code generator, I think.
> 
> I think I like that we're black listing (rather than white listing) though.  That way people adding constants in the future will need to opt out rather than in.

I'm not entirely convinced it's a property of the interface/idl to decide whether or not to check the generated code: the way I see, the code generator is the bit that knows about the details of the generated code and uses the c++ class, not the idl. Anyways, no strong feelings either way.

I added a new property "DontCheckEnums" and spread across the various interfaces where necessary, and also move the code up to CodeGenerator.pm so that both JS and V8 are now sharing this check. another look please?

[Jeremy Orlow]

Comment on attachment 61523 [details]
Patch

WebCore/dom/OverflowEvent.idl:26
 +      # FIXME: Remove DontCheckEnums attribute.
Remove it when?

WebCore/svg/SVGGradientElement.idl:28
 +      interface [Conditional=SVG, DontCheckEnums] SVGGradientElement : SVGElement,
Do wrapping like some of the earlier interfaces in this file.



close

[Marcus Bulach]

Created attachment 61528 [details]
Patch

[Marcus Bulach]

thanks Jeremy!

(In reply to comment #8)
> (From update of attachment 61523 [details])
> WebCore/dom/OverflowEvent.idl:26
>  +      # FIXME: Remove DontCheckEnums attribute.
> Remove it when?
clarified the comment.

> 
> WebCore/svg/SVGGradientElement.idl:28
>  +      interface [Conditional=SVG, DontCheckEnums] SVGGradientElement : SVGElement,
> Do wrapping like some of the earlier interfaces in this file.
done (and tidy up a few others while at it..).

> 
> 
> 
> close

[WebKit Review Bot]

Attachment 61528 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1250:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1255:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1260:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1265:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1270:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:178:  jsTestObjCONST_VALUE_0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:179:  jsTestObjCONST_VALUE_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:180:  jsTestObjCONST_VALUE_2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:181:  jsTestObjCONST_VALUE_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.h:182:  jsTestObjCONST_VALUE_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1026:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 12 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Orlow]

Comment on attachment 61528 [details]
Patch

r=me

[Darin Adler]

Why do we need to disable the checking in so many of the existing IDL files? Is it one reason? Multiple reasons? Before we make this the way we do everything in the future, I’d like to understand exactly why our existing code isn’t conformant.

[Marcus Bulach]

(In reply to comment #13)
> Why do we need to disable the checking in so many of the existing IDL files? Is it one reason? Multiple reasons? Before we make this the way we do everything in the future, I’d like to understand exactly why our existing code isn’t conformant.

Hi Darin,

There are a few reasons, here's a sample:
1. Node.idl / .h: some of the enums are defined are defined as global constants rather than enums in the .h (DOCUMENT_POSITION_*)

2. DOMCoreException (and a few SVG-related idls): the enum is not defined in the corresponding .h.

3. RangeException.idl / .h: in the header file, the actual constant values are defined in terms of some other value:
BAD_BOUNDARYPOINTS_ERR = RangeExceptionOffset + 1;

4. one other case, OverflowEvent, the values simply don't match.

on 1 and 4, I'm happy to chase this and clean it up, I think it's reasonably straightforward. For (2), I think we'd need to keep the check. For (3), I'm not familiar with those bits, so I'm not sure yet whether or not it's possible to fix.

Please, let me know if this is any clear.

[Darin Adler]

In my opinion, any that are easy to fix should be fixed in the initial check-in. It’s fine to leave some undone -- my concern is that there are 31 uses of it here.

[WebKit Commit Bot]

Comment on attachment 61528 [details]
Patch

Clearing flags on attachment: 61528

Committed r63331: <http://trac.webkit.org/changeset/63331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Marcus Bulach]

(In reply to comment #15)
> In my opinion, any that are easy to fix should be fixed in the initial check-in. It’s fine to leave some undone -- my concern is that there are 31 uses of it here.

Apologies Darin, this landed through the commit queue.
I'll do a follow up and will cc: you.

[Tony Gentilcore]

(In reply to comment #18)
> (In reply to comment #15)
> > In my opinion, any that are easy to fix should be fixed in the initial check-in. It’s fine to leave some undone -- my concern is that there are 31 uses of it here.
> 
> Apologies Darin, this landed through the commit queue.
> I'll do a follow up and will cc: you.

Would you mind copying me too? I need to update WebCore/page/Navigation.{idl|h}, but it isn't immediately clear to me what the correct pattern is.

[Darin Adler]

It’s bug 42278.

[Shinichiro Hamaji]

Committed r63736: <http://trac.webkit.org/changeset/63736>