Bug 41613 – Adds support for generating code from IDL with reserved keywords (it'll be necessary for IDBCursor::continue).

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

[Marcus Bulach]

Adds support for generating code from IDL with reserved keywords (it'll be necessary for IDBCursor::continue).

[Marcus Bulach]

Created attachment 60529 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:681:  WebDOMTestObj::continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:689:  WebDOMTestObj::delete_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:124:  continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:125:  delete_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 17 files


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

[Marcus Bulach]

jeremy: this patch will allow IDBCursor to have its infamous "continue" method.
Two questions though:

1. we could probably get away with a [Custom] binding, but I think it's better to improve the generator?

2. this patch simply adds a "_" suffix for a couple of reserved keywords. please, let me know if you think of a better idea to escape such names.

[Jeremy Orlow]

Comment on attachment 60529 [details]
Patch

Looks good, except the way you're escaping it doesn't seem very webkitty.  Not sure what'd be a better suffix tho.  :-(

Also, you should git blame this and cc some others who work in this code.

[Marcus Bulach]

[e-mail]

Nate, I guess you know quite a lot of the code generators, would you mind taking a look at this?

quick background: we're working on IndexedDB, and some of its IDLs use C++ reserved keywords such "continue" and "delete" on methods such as "IDBCursor::continue()".
The idea is that we need to escape such methods somehow, and in this patch I used a "_" as a suffix.. would you have any suggestion?

[Marcus Bulach]

[e-mail]

Adam, this is the part of naming mangling / escaping you commented on https://bugs.webkit.org/show_bug.cgi?id=41888.

I'd be happy to hear your ideas here as well. Having the IDL attribute as you suggested, "[ImplementationName=continue_]", could help clarifying, but I'm afraid it just adds more "noise" and the real problem (how to define the implementation name) still exists and it'd be solved ad hoc per method.

I think it'd be better if we could define an automated mangling/escaping mechanism. I borrowed the current idea from http://www.w3.org/TR/WebIDL/#java-binding (although I'm using a suffix rather than prefix). I'm obviously happy to go any other way.

Thoughts?

[Adam Barth]

My thought about using an IDL attribute arise from the general design principle of being explicit rather than implicit.  As for adding noise, I think it is much less common than some of our other attributes.  :)

[Jeremy Orlow]

(In reply to comment #7)
> My thought about using an IDL attribute arise from the general design principle of being explicit rather than implicit.  As for adding noise, I think it is much less common than some of our other attributes.  :)

Generally I'd agree with you Adam, but in this case my thinking is that these are reserved words and thus there is never a reason why they shouldn't be changed to something else.

I guess we could have the generator raise an error if it sees one without such an attribute?

[Adam Barth]

> I guess we could have the generator raise an error if it sees one without such an attribute?

Well, the compiler will happily raise that error for you.  :)

[Marcus Bulach]

(In reply to comment #7)
> My thought about using an IDL attribute arise from the general design principle of being explicit rather than implicit.  As for adding noise, I think it is much less common than some of our other attributes.  :)

I agree that it'd be explicit, but it wouldn't really solve the problem, right? :)

I mean, we'd still need to have a rule on how to define the escaping/mangling, and then if we have such a rule, why not just have it written in code? :)

Perhaps a compromise would be just have an attribute like "[EscapeCppName]" in the IDL (making it explicit) but then letting the codegenerator (making it normative) do the actual escaping? how does that sound?

[Jeremy Orlow]

(In reply to comment #9)
> > I guess we could have the generator raise an error if it sees one without such an attribute?
> 
> Well, the compiler will happily raise that error for you.  :)

For C++ reserved words.  The worse case is JS or other binding language reserved words in which case the error might only be seen in run time.

[Adam Barth]

> Perhaps a compromise would be just have an attribute like "[EscapeCppName]" in the IDL (making it explicit) but then letting the codegenerator (making it normative) do the actual escaping? how does that sound?

Yeah, something like [WebCoreName=continueDarnIt]

> For C++ reserved words.  The worse case is JS or other binding language reserved words in which case the error might only be seen in run time.

I feel like we should deal with that problem when we come to it.  Currently, all the code generators generate C++ code.

[Adam Barth]

BTW, Mozilla has such an attribute in their IDL language as well.

[Marcus Bulach]

(In reply to comment #12)
> > Perhaps a compromise would be just have an attribute like "[EscapeCppName]" in the IDL (making it explicit) but then letting the codegenerator (making it normative) do the actual escaping? how does that sound?
> 
> Yeah, something like [WebCoreName=continueDarnIt]

hmm, but in this case the "DarnIt" suffix would be ad hoc..

granted, it wouldn't be that common, but I'd prefer to establish an enforced rule in the generator rather than discuss what suffix should be used.
just to be clear, I'm happy with DarnIt ;) but the attribute should be [EscapeWebCoreNameDarnIt] :)
no options allowed, more straightforward code reviews.
how does it sound?

> 
> > For C++ reserved words.  The worse case is JS or other binding language reserved words in which case the error might only be seen in run time.
> 
> I feel like we should deal with that problem when we come to it.  Currently, all the code generators generate C++ code.

[Adam Barth]

I feel like we're over designing this feature.  We should just take the implementation name as the value of the attribute.

[Jeremy Orlow]

For what it's worth, I think the original approach Marcus had was fine and that making this more general purpose was an "overdesign" as well.  So I think it's a bit unfair to dismis Marcus' suggestion as "overdesign".

That said, I would lean towards the [WebCoreName=continueForRealz] approach.

[Adam Barth]

> So I think it's a bit unfair to dismis Marcus' suggestion as "overdesign".

Oh, I didn't mean to dimiss Marcus' suggestion.  I just meant that we should just pick something and go with it, whether that's the implicit design, the parameterized explicit design, or the unparameterized explicit design.

[Marcus Bulach]

(In reply to comment #17)
> > So I think it's a bit unfair to dismis Marcus' suggestion as "overdesign".
> 
> Oh, I didn't mean to dimiss Marcus' suggestion.  I just meant that we should just pick something and go with it, whether that's the implicit design, the parameterized explicit design, or the unparameterized explicit design.

:)
my votes, in order of preference:
1. unparameterized explicit design
2. parameterized explicit design
3. implicit design

I'm happy with whatever, please cast your votes.. but more important: any suggestion for the escaping rule? :)

[Jeremy Orlow]

You could (In reply to comment #18)
> (In reply to comment #17)
> > > So I think it's a bit unfair to dismis Marcus' suggestion as "overdesign".
> > 
> > Oh, I didn't mean to dimiss Marcus' suggestion.  I just meant that we should just pick something and go with it, whether that's the implicit design, the parameterized explicit design, or the unparameterized explicit design.
> 
> :)
> my votes, in order of preference:
> 1. unparameterized explicit design
> 2. parameterized explicit design
> 3. implicit design
> 
> I'm happy with whatever, please cast your votes.. but more important: any suggestion for the escaping rule? :)

Function?  i.e.  "continueFunction" or "deleteFunction"?

[Adam Barth]

Yay design by committee.  :)

1. parameterized explicit design
2. unparameterized explicit design
3. implicit design

[Marcus Bulach]

(In reply to comment #20)
> Yay design by committee.  :)
> 
> 1. parameterized explicit design
> 2. unparameterized explicit design
> 3. implicit design

yay, decided! :) parametrized explicit design, and I'll use "Function" as suffix on the baseline TestObj.idl and the methods I'll need on the IDBCursor.. new patch follow soon. thanks both!

[Marcus Bulach]

(In reply to comment #21)
> (In reply to comment #20)
> > Yay design by committee.  :)
> > 
> > 1. parameterized explicit design
> > 2. unparameterized explicit design
> > 3. implicit design
> 
> yay, decided! :) parametrized explicit design, and I'll use "Function" as suffix on the baseline TestObj.idl and the methods I'll need on the IDBCursor.. new patch follow soon. thanks both!

duh :(
apparently there's already support for *exactly* this.

http://trac.webkit.org/browser/trunk/WebCore/page/Console.idl
[CustomArgumentHandling, ImplementationFunction=assertCondition] void assert(in boolean condition);

I'll double check it works and hopefully close this issue.
thanks for all your help, apologies for wasting your time.

[Marcus Bulach]

Attribute [ImplementationFunction=nameFunction] does exactly what this was intending. Closing this as invalid.