Support Dart optional arguments which are not dispatched, but instead use default values.

Support Dart optional arguments which are not dispatched, but instead use default values.

BUG=1886
Committed: https://code.google.com/p/dart/source/detail?r=4871

[podivilov, 2012-02-28 17:46:23]

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
File client/dom/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:133:
argument.ext_attrs['DartConvertNullToDefaultValue'] = None
DartConvertNullToDefaultValue name doesn't reflect actual usage. Maybe
RequiredInCPP or something like that?

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:134: continue
nit: elif

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
File client/dom/scripts/systemwrapping.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:482: return False
Could you please move this to another block, so it doesn't mess with (type,
test, pred) logic?

[antonm, 2012-02-28 19:22:08]

Pavel, thanks a lot for review.  I've punted on all your comments just to have
another iteration---if you insist, I'll follow your advice.

Stephen, I'd really appreciate your opinion on CL and controversial points.

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
File client/dom/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:133:
argument.ext_attrs['DartConvertNullToDefaultValue'] = None
On 2012/02/28 17:46:24, podivilov wrote:
> DartConvertNullToDefaultValue name doesn't reflect actual usage. Maybe
> RequiredInCPP or something like that?

I am on the fence here.  To some extent this describes exactly what goes on (but
not why) and matches perfectly to conversion flags (which can be changed too, of
course).

Stephen, any thoughts?

Overall, I'd be glad to rename if you think it's a good idea---no personal
attachment to this wording :)

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:134: continue
On 2012/02/28 17:46:24, podivilov wrote:
> nit: elif

I'd prefer continue as it clearly says processing of this argument is over while
with elif one may expect some code after if.

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
File client/dom/scripts/systemwrapping.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:482: return False
On 2012/02/28 17:46:24, podivilov wrote:
> Could you please move this to another block, so it doesn't mess with (type,
> test, pred) logic?

Again, I'll do that, but first I'll try to object :)

This is a special case when we meet a special kind of optional.  It naturally
belongs to this else clause, but, of course, I can repeat if condition above.

Once again, Stephen, I'd be happy to learn what you think about it and as you
both have ways more experience with this code base, I'll follow your advice.

[sra1, 2012-02-28 22:04:42]

My 2к

I'm LGTM if you agree.

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
File client/dom/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:133:
argument.ext_attrs['DartConvertNullToDefaultValue'] = None
On 2012/02/28 19:22:08, antonm wrote:
> On 2012/02/28 17:46:24, podivilov wrote:
> > DartConvertNullToDefaultValue name doesn't reflect actual usage. Maybe
> > RequiredInCPP or something like that?
> 
> I am on the fence here.  To some extent this describes exactly what goes on
(but
> not why) and matches perfectly to conversion flags (which can be changed too,
of
> course).
> 
> Stephen, any thoughts?
> 
> Overall, I'd be glad to rename if you think it's a good idea---no personal
> attachment to this wording :)

What is confusing to me is that 'null' and 'default value' are kind of the same
thing.  The first 'null' might be a default value or an explicit null (we can
never know).  The second 'default value' is an explicit value rather than using
the default value (not) available in C++.

Is this issue Dart specific (i.e. needs to be done this way for Frog), or native
only?  The name should reflect that.

DartiumConvertNullToExplicitDefaultValue ?
DartiumRequiresExplicitDefaultValue ?
CPPRequiresExplicitDefaultValue ?

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:134: continue
On 2012/02/28 19:22:08, antonm wrote:
> On 2012/02/28 17:46:24, podivilov wrote:
> > nit: elif
> 
> I'd prefer continue as it clearly says processing of this argument is over
while
> with elif one may expect some code after if.

+1 for continue

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
File client/dom/scripts/systemwrapping.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:472: def IsConvertNullToDefaultValue(arg):
return 'DartConvertNullToDefaultValue' in arg.ext_attrs
Lines too long

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:482: return False
On 2012/02/28 19:22:08, antonm wrote:
> On 2012/02/28 17:46:24, podivilov wrote:
> > Could you please move this to another block, so it doesn't mess with (type,
> > test, pred) logic?
> 
> Again, I'll do that, but first I'll try to object :)
> 
> This is a special case when we meet a special kind of optional.  It naturally
> belongs to this else clause, but, of course, I can repeat if condition above.
> 
> Once again, Stephen, I'd be happy to learn what you think about it and as you
> both have ways more experience with this code base, I'll follow your advice.

So it possible (at least theoretically) to have a dispatch AND an argument that
is DartConvertNullToDefaultValue 
E.g.
  foo(int x, [DartConvertNullToDefaultValue, Optional] y);
  foo(String s, [DartConvertNullToDefaultValue, Optional] z);

If so, the test is in the 'right' place but it is unfortunate that so much of
the machinery of the dispatch generation surrounds it.

I would move it back up,  and factor out the predicate, so it reads something
like this:

if self._SingleDispatchDueToForcedPassingOfOptionalValues(position, ..., ):
  self._GenerateSingle...
  return False

[antonm, 2012-02-29 10:29:40]

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
File client/dom/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/d...
client/dom/scripts/databasebuilder.py:133:
argument.ext_attrs['DartConvertNullToDefaultValue'] = None
This shouldn't be Dartium specific and should apply to JS backends as well, for
example, in those cases there is no need to do any dispatch (and you probably do
not do it anyway).

Yes, I am agree that name is somewhat confusing here and I am glad to rename it.
 Maybe reflect its semantics in our codegenerator?  Something like
DartNoDispatchOptional?  or DartDefaultValueOptional?

BTW, there is a simple trick which can allow us to provide very true optionals:

we can add _OPTIONAL = const Object(); into domimpl and make all optionals
default to it.  Now we can distinguish it from null and client code cannot pass
this fake value.

On 2012/02/28 22:04:42, sra1 wrote:
> On 2012/02/28 19:22:08, antonm wrote:
> > On 2012/02/28 17:46:24, podivilov wrote:
> > > DartConvertNullToDefaultValue name doesn't reflect actual usage. Maybe
> > > RequiredInCPP or something like that?
> > 
> > I am on the fence here.  To some extent this describes exactly what goes on
> (but
> > not why) and matches perfectly to conversion flags (which can be changed
too,
> of
> > course).
> > 
> > Stephen, any thoughts?
> > 
> > Overall, I'd be glad to rename if you think it's a good idea---no personal
> > attachment to this wording :)
> 
> What is confusing to me is that 'null' and 'default value' are kind of the
same
> thing.  The first 'null' might be a default value or an explicit null (we can
> never know).  The second 'default value' is an explicit value rather than
using
> the default value (not) available in C++.
> 
> Is this issue Dart specific (i.e. needs to be done this way for Frog), or
native
> only?  The name should reflect that.
> 
> DartiumConvertNullToExplicitDefaultValue ?
> DartiumRequiresExplicitDefaultValue ?
> CPPRequiresExplicitDefaultValue ?

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
File client/dom/scripts/systemwrapping.py (right):

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:472: def IsConvertNullToDefaultValue(arg):
return 'DartConvertNullToDefaultValue' in arg.ext_attrs
On 2012/02/28 22:04:42, sra1 wrote:
> Lines too long

Done.

https://chromiumcodereview.appspot.com/9495014/diff/3001/client/dom/scripts/s...
client/dom/scripts/systemwrapping.py:482: return False
On 2012/02/28 22:04:42, sra1 wrote:
> On 2012/02/28 19:22:08, antonm wrote:
> > On 2012/02/28 17:46:24, podivilov wrote:
> > > Could you please move this to another block, so it doesn't mess with
(type,
> > > test, pred) logic?
> > 
> > Again, I'll do that, but first I'll try to object :)
> > 
> > This is a special case when we meet a special kind of optional.  It
naturally
> > belongs to this else clause, but, of course, I can repeat if condition
above.
> > 
> > Once again, Stephen, I'd be happy to learn what you think about it and as
you
> > both have ways more experience with this code base, I'll follow your advice.
> 
> So it possible (at least theoretically) to have a dispatch AND an argument
that
> is DartConvertNullToDefaultValue 
> E.g.
>   foo(int x, [DartConvertNullToDefaultValue, Optional] y);
>   foo(String s, [DartConvertNullToDefaultValue, Optional] z);

Yes, IDBDatabase.transaction is exactly the example.

> 
> If so, the test is in the 'right' place but it is unfortunate that so much of
> the machinery of the dispatch generation surrounds it.
> 
> I would move it back up,  and factor out the predicate, so it reads something
> like this:
> 
> if self._SingleDispatchDueToForcedPassingOfOptionalValues(position, ..., ):
>   self._GenerateSingle...
>   return False

Ok, what you both think about the following variant?

[antonm, 2012-03-01 14:51:45]

PTAL

As discussed offline, renamed to RequiredCppParameter.

[podivilov, 2012-03-02 10:04:02]

lgtm