| Summary: | Code generator: ensure generated constants match their corresponding enums. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Marcus Bulach <bulach> | ||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, darin, hamaji, japhet, jorlow, tonyg, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Other | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Marcus Bulach
2010-07-14 05:19:01 PDT
Created attachment 61507 [details]
Patch
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.
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. 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.
Created attachment 61523 [details]
Patch
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.
(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? 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
Created attachment 61528 [details]
Patch
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 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.
Comment on attachment 61528 [details]
Patch
r=me
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. (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. 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. Comment on attachment 61528 [details] Patch Clearing flags on attachment: 61528 Committed r63331: <http://trac.webkit.org/changeset/63331> All reviewed patches have been landed. Closing bug. (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. (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. It’s bug 42278. Committed r63736: <http://trac.webkit.org/changeset/63736> |