Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10387170:
New dispatch from dom into native code. (Closed)
|
Created:
8 years, 7 months ago by sra1 Modified:
8 years, 7 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionDispatch changes to be more like V8.
Overloads are tested in turn, and overloads that differ only by optional arguments are grouped into one native method where possible.
Committed: https://code.google.com/p/dart/source/detail?r=7974
Patch Set 1 : #
Total comments: 12
Patch Set 2 : rebase #Patch Set 3 : validate RequireCppParameter #Patch Set 4 : rebase #Patch Set 5 : Avoid unimplemented types in combined overloads #Patch Set 6 : rebase #
Total comments: 14
Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : rebase #
Total comments: 11
Messages
Total messages: 6 (0 generated)
I have not tested this yet - I have leave early for the day.
The main question - is it doing the right thing?
The assumption is that the optional arguments are defaulted properly in the
native code when null is passed.
Note at the moment we consider all inputs to be nullable for the dispatch.
We can make some types non-nullable (num, int, bool, String??), and if we do, we
should probably verify the non-optional arguments.
The dispatch is very basic and could do with some optimization but we can do
that later. There will be more need for optimization if we do this. Or perhaps
the VM is smart enough?
Old:
IDBRequest openCursor([key_OR_range = null, direction = null]) {
if (key_OR_range is IDBKeyRange) {
if (direction === null) {
return _openCursor(key_OR_range);
} else {
if (direction is String) {
return _openCursor_2(key_OR_range, direction);
} else {
if (direction is int) {
return _openCursor_3(key_OR_range, direction);
}
}
}
} else {
if (direction === null) {
return _openCursor_4(key_OR_range);
} else {
if (direction is String) {
return _openCursor_5(key_OR_range, direction);
} else {
if (direction is int) {
return _openCursor_6(key_OR_range, direction);
}
}
}
}
throw "Incorrect number or type of arguments";
}
New:
IDBRequest openCursor([key_OR_range = null, direction = null]) {
//
// openCursor([Optional] IDBKeyRange range)
// openCursor([Optional] IDBKeyRange range, [Optional] String direction)
// openCursor(Dynamic key)
// openCursor(Dynamic key, [Optional] String direction)
// openCursor(IDBKeyRange range, int direction)
// openCursor(Dynamic key, int direction)
//
// -- reduced:
// openCursor([Optional] IDBKeyRange range)
// openCursor([Optional] IDBKeyRange range, [Optional] String direction)
// openCursor(Dynamic key, [Optional] String direction)
// openCursor(IDBKeyRange range, int direction)
// openCursor(Dynamic key, int direction)
//
if ((key_OR_range === null || key_OR_range is IDBKeyRange) &&
direction === null) {
return _openCursor(key_OR_range);
}
if ((key_OR_range === null || key_OR_range is IDBKeyRange) &&
(direction === null || direction is String)) {
return _openCursor_2(key_OR_range, direction);
}
if ((direction === null || direction is String)) {
return _openCursor_3(key_OR_range, direction);
}
if ((key_OR_range === null || key_OR_range is IDBKeyRange) &&
(direction === null || direction is int)) {
return _openCursor_4(key_OR_range, direction);
}
if ((direction === null || direction is int)) {
return _openCursor_5(key_OR_range, direction);
}
throw "Incorrect number or type of arguments";
}
New if we do not suppress openCursor(), the first three can be merged:
IDBRequest openCursor([key_OR_range = null, direction = null]) {
//
// openCursor()
// openCursor([Optional] IDBKeyRange range)
// openCursor([Optional] IDBKeyRange range, [Optional] String direction)
// openCursor(Dynamic key)
// openCursor(Dynamic key, [Optional] String direction)
// openCursor(IDBKeyRange range, int direction)
// openCursor(Dynamic key, int direction)
//
// -- reduced:
// openCursor([Optional] IDBKeyRange range, [Optional] String direction)
// openCursor(Dynamic key, [Optional] String direction)
// openCursor(IDBKeyRange range, int direction)
// openCursor(Dynamic key, int direction)
//
if ((key_OR_range === null || key_OR_range is IDBKeyRange) &&
(direction === null || direction is String)) {
return _openCursor(key_OR_range, direction);
}
if ((direction === null || direction is String)) {
return _openCursor_2(key_OR_range, direction);
}
if ((key_OR_range === null || key_OR_range is IDBKeyRange) &&
(direction === null || direction is int)) {
return _openCursor_3(key_OR_range, direction);
}
if ((direction === null || direction is int)) {
return _openCursor_4(key_OR_range, direction);
}
throw "Incorrect number or type of arguments";
}
[+Pavel] I like overall approach and thanks a lot for working on it. I really hate allow nulls in, maybe we should restrict them only to overloads, esp. given that we may want to switch to sentinels for defaults later, but I am not sure if it's worth the effort, esp. as I assume dart2js won't do null checks anyway, and we probably should behave here similarly, correct? https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... File lib/dom/scripts/systemnative.py (right): https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:848: def CombineOverloads(self, overloads): that still looks weird to me to recombine overloads after expansion, but, ok, up to you https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:870: if probe.arguments[0:-1] != prev.arguments: nit: probe.arguments[:-1] should do too. https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:925: matches = filter(IsRequiredCppParameter, op.arguments) that is somewhat complicated to read, maybe use itertools.dropwhile? https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:953: for position in range(0, len(info.param_infos)): nit: for (position, param) in enumerate(info.param_infos): ? https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:957: type = DartType(arg.type.id) nit: dart_type instead of type? https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:958: if type == param.dart_type: this check looks somewhat hacky, maybe it should be something like type != 'Dynamic'? https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:970: if len(cond) + len(indent) + 7 > 80: :)
PTAL I had to work around an issue with incomplete code (see new Issue 3177) https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... File lib/dom/scripts/systemnative.py (right): https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:848: def CombineOverloads(self, overloads): On 2012/05/18 11:33:40, antonmuhin wrote: > that still looks weird to me to recombine overloads after expansion, but, ok, up > to you Think of it as an optimization. Without it, the generated code is bigger, there are more native methods, but the types of the extracted arguments in the extra native methods are the same, so the final functioning is the same. https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:870: if probe.arguments[0:-1] != prev.arguments: On 2012/05/18 11:33:40, antonmuhin wrote: > nit: probe.arguments[:-1] should do too. Done. https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:953: for position in range(0, len(info.param_infos)): On 2012/05/18 11:33:40, antonmuhin wrote: > nit: for (position, param) in enumerate(info.param_infos): ? Done. https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:957: type = DartType(arg.type.id) On 2012/05/18 11:33:40, antonmuhin wrote: > nit: dart_type instead of type? Done. https://chromiumcodereview.appspot.com/10387170/diff/2002/lib/dom/scripts/sys... lib/dom/scripts/systemnative.py:958: if type == param.dart_type: On 2012/05/18 11:33:40, antonmuhin wrote: > this check looks somewhat hacky, maybe it should be something like type != > 'Dynamic'? Done.
https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... File lib/dom/scripts/systemnative.py (right): https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:843: if info.name in ['createObjectStore']: why? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:851: def CombineOverloads(self, overloads): what about the following implementation: out = [overloads[-1]] for overload in overloads[1::-1]: if not implements(overload, out[-1]): out.append(overload) def implements(overload, another): return overload.arguments == another.arguments[:len(overload.arguments) and all((arg.is_optional for arg in another.arguments[len(overload.arguments):])) + Plus additional hack for unimplemented https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:877: # See Issue 3177. This test against known implemented types is to I suspect we need some more amendments in bindings: we should be able to deal with nulls and that should solve the issue with 'unimplemented' you mention here, shouldn't it? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:935: def HasRequiredCppParameters(op): first_required = itertools.dropwhile(lambda x: not IsRequiredCppParameter(x), op.arguments) if not all((IsRequiredCppParameter(arg) for arg in first_required)): raise ... ??? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:946: if any(HasRequiredCppParameters(op) for op in overloads): should it be any or longest? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:950: for (index, arg) in enumerate(op.arguments): for (index, (arg1, arg2)) in enumerate(zip(op.arguments, longest.arguments)): ??? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:978: if position < len(operation.arguments): why special case last argument? https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:981: if dart_type == param.dart_type: what does this test check?
https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... File lib/dom/scripts/systemnative.py (right): https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:843: if info.name in ['createObjectStore']: On 2012/05/23 14:43:33, antonmuhin wrote: > why? Sorry, debugging. removed https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:935: def HasRequiredCppParameters(op): On 2012/05/23 14:43:33, antonmuhin wrote: > first_required = itertools.dropwhile(lambda x: not IsRequiredCppParameter(x), > op.arguments) > > if not all((IsRequiredCppParameter(arg) for arg in first_required)): > raise ... > > ??? This does not turn out so nice: unlike a list, the iterator gets exhausted by the all-test or any attempt to see if it is empty. An empty iterator can be truthy. https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:946: if any(HasRequiredCppParameters(op) for op in overloads): On 2012/05/23 14:43:33, antonmuhin wrote: > should it be any or longest? any, in case we have two unrelated overloads. https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:950: for (index, arg) in enumerate(op.arguments): On 2012/05/23 14:43:33, antonmuhin wrote: > for (index, (arg1, arg2)) in enumerate(zip(op.arguments, longest.arguments)): > > ??? Does not really shorten it since we still have to pull the types of the IDLArguments. https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:978: if position < len(operation.arguments): On 2012/05/23 14:43:33, antonmuhin wrote: > why special case last argument? This is the out-of-arguments check. position == len(operation.arguments) would read past end of list in next line. https://chromiumcodereview.appspot.com/10387170/diff/10002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:981: if dart_type == param.dart_type: On 2012/05/23 14:43:33, antonmuhin wrote: > what does this test check? It checks if a test is necessary. It catches the common case where the method parameter type is the overload parameter type, always true where there is one overload.
https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... File lib/dom/scripts/systemnative.py (right): https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:884: if len(seed.arguments) > 0 and seed.arguments[-1].is_optional: when this condition fires? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:897: if probe.arguments[:-1] != prev.arguments: that should cover the condition above, no? as we cannot have two no args methods following? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:901: # See Issue 3177. This test against known implemented types is to Stephen, I don't think we need this code, we should support nulls in conversions. https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:965: if len(matches) != len(rematches): okay, but here if not all(IsRequiredCppParameter, op.arguments[-len(matches)]): looks more idiomatic, no? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1006: if dart_type == param.dart_type: won't it be more straightforward to check param.default_value is None? maybe adding a corresponding helper method to ParamInfo? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1014: test = TypeCheck(param.name, dart_type) we have single call site for TypeCheck/NullCheck, shouldn't they get inlined? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1016: test = '(%s || %s)' % (NullCheck(param.name), test) any chances it might be incorrect in the following weird declaration: foo([Optional] X x, [Optional] Y y); foo([Optional] X x, [Optional] Z z); here for x you'll have None as test, correct? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1019: if test: why this check, cannot you populate tests just in place? https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1021: if tests: Please, add a blank line https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1023: if len(cond) + len(indent) + 7 > 80: I wouldn't complicate this logic to keep 80 constraint in generated files. https://chromiumcodereview.appspot.com/10387170/diff/13002/lib/dom/scripts/sy... lib/dom/scripts/systemnative.py:1034: fallthrough = False you probably want to return from here, correct, maybe throw exception if there are overloads to resolve? overall, do we need fallthrough local? |
