More multi-version support

More multi-version support

Minor fix to idl_output: switch from None to '' to alow split to work
Add 'InReleases' member to verify node is valid within a list of releases.
Add 'releases' member to IDLVersionMap class for easier search.
Remove stale code and build ordered list of requested releases in AST
Remove release member from CGen object, and pass into functions
Move release tracking from C prototype to C header generator
Update generator tests.
Added golden files to verify the C generator is correct

NOTE:  This is work in progress.  The header emitted using
--release=X should be correct and has been tested to generate
exactly what we have today, however --range=X,Y does not yet
generate what we need.

BUG= http://code.google.com/p/chromium/issues/detail?id=89969
TEST= python generator.py
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98133

[dmichael (off chromium), 2011-08-24 20:49:11]

Still looking, but here are some early comments.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py
File ppapi/generators/idl_c_proto.py (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:31: if lines[-1] == '':
I guess my python-fu is weak. What does indexing at -1 do? Is there a clearer
way to do it?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:34: out = '%s/*' % tab + ('\n%s *' %
tab).join(lines) + ' */\n'
Maybe a comment on what's going on here? And maybe do the common stuff in one
place, and only do the last part in a conditional.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:37: def Comment(node, prefix=None, tabs=0):
Speaking of comments...  Could we have some here? :-) I don't find it obvious
what is happening here. The comment is the name of the node? prefix can have
carriage returns? What does that do?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:41: prefix = prefix.split('\n')
I'd prefer you use a different variable name for the list instead of
over-writing prefix (which was presumably passed as a string)

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:134: #    self.release = GetOption('release')
Why are these just commented out? If they're dead, please delete them. Same
below.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:184: # For Members, Params, and Typedef's your
want type it refers to
Typedef's->Typedefs
and not sure what 'your want type it refers to' is supposed to say.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:355: # Define an Typedef.
nit: an->a

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:356: def DefineTypedef(self, node, releases,
prefix='', comment=False):
Why are you passing an array here if you only use the first element? Is this
transitional?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:364: def DefineEnum(self, node, releases,
prefix='', comment=False):
releases appears to be unused here

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:390: release = releases[0]
Again, is this just temporary?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:417: def DefineType(self, node, releases,
prefix='', comment=False):
Why the trivial function? Is this speculative coding that should be deleted?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:455: out += '%s%s' % (comment_txt, define_txt)
Doesn't the % operator seem like overkill here?
out += comment_txt + define_text

Or the whole thing could be:
if comment_txt and comment:
  out += comment_txt
out += define_txt

Maybe this is a matter of preference, but it seems simpler to me to do the
unconditional stuff unconditionally.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:463: line = '%s%s' % (tab, line)
again with the percent operator for just concatenation...  is this a
Google/Python style thing I just haven't been indoctrinated in to yet? If it's
the 'normal' way, then fine. Just seems strange to me.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:476: space = ' '.join(['' for i in range(left)])
Why not just
' ' * (left-1)
Seems clearer to me, anyway.

[dmichael (off chromium), 2011-08-24 21:15:29]

Done reviewing until the next patch.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_generator.py
File ppapi/generators/idl_generator.py (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_generator....
ppapi/generators/idl_generator.py:119: class GeneratorByFile(Generator):
Could you add a brief comment for the class?

By the way, in the future, you should default to doing docstrings for class and
method documentation (as opposed to just #)

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
File ppapi/generators/test_cgen_range/versions.h (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:15: * File Comment. */
Looks like a problem in generating that comment...  shouldn't the */ be on the
next line?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:22: /* Bogus Struct */
Should the comment become a doxygen-style comment? Or did it have to be one in
the idl file for that to happen?

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:35: int32_t (*Foo)(int32_t x,
int32_t y, int32_t z);
Maybe you can throw a comment somewhere (wherever makes sense) that this is a
work in progress...  as you know, the above code isn't right for Chrome or the
public-facing headers yet. I don't want to ask you to fix all that in this CL,
but it would be good to have a quick comment somewhere to that effect.

[noelallen1, 2011-08-24 22:43:06]

Fixed nits and missing comments.
As noted, use of releases is still not complete, more CLs to follow to complete
support of multiple versions.
However using
$ ./generator.py --wnone --cgen --test --diff

Shows that the new generate produced the same results as what is currently
checked into src/ppapi/c which is effectively my "golden files" for this
transition.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py
File ppapi/generators/idl_c_proto.py (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:31: if lines[-1] == '':
On 2011/08/24 20:49:11, dmichael wrote:
> I guess my python-fu is weak. What does indexing at -1 do? Is there a clearer
> way to do it?
[-n] is the standard way to notate the 'nth' to last element.  But a more
correct way may be to do:
if not lines[-1]
Either way I will comment.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:34: out = '%s/*' % tab + ('\n%s *' %
tab).join(lines) + ' */\n'
On 2011/08/24 20:49:11, dmichael wrote:
> Maybe a comment on what's going on here? And maybe do the common stuff in one
> place, and only do the last part in a conditional.

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:37: def Comment(node, prefix=None, tabs=0):
On 2011/08/24 20:49:11, dmichael wrote:
> Speaking of comments...  Could we have some here? :-) I don't find it obvious
> what is happening here. The comment is the name of the node? prefix can have
> carriage returns? What does that do?

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:41: prefix = prefix.split('\n')
On 2011/08/24 20:49:11, dmichael wrote:
> I'd prefer you use a different variable name for the list instead of
> over-writing prefix (which was presumably passed as a string)

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:134: #    self.release = GetOption('release')
On 2011/08/24 20:49:11, dmichael wrote:
> Why are these just commented out? If they're dead, please delete them. Same
> below.

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:184: # For Members, Params, and Typedef's your
want type it refers to
On 2011/08/24 20:49:11, dmichael wrote:
> Typedef's->Typedefs
> and not sure what 'your want type it refers to' is supposed to say.

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:356: def DefineTypedef(self, node, releases,
prefix='', comment=False):
On 2011/08/24 20:49:11, dmichael wrote:
> Why are you passing an array here if you only use the first element? Is this
> transitional?

Yes.  This is waiting on name mangling based on version which will be the next
CL.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:364: def DefineEnum(self, node, releases,
prefix='', comment=False):
On 2011/08/24 20:49:11, dmichael wrote:
> releases appears to be unused here

Transitional (same as above)

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:390: release = releases[0]
On 2011/08/24 20:49:11, dmichael wrote:
> Again, is this just temporary?
Yes.   All Define<X> functions take an array of releases to generate.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:417: def DefineType(self, node, releases,
prefix='', comment=False):
On 2011/08/24 20:49:11, dmichael wrote:
> Why the trivial function? Is this speculative coding that should be deleted?

A 'type' doesn't have anything to emit for C header generator, but would be
needed for the C Test generator.  I will remove for now and add it back if/when
I need it for a different generator.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:455: out += '%s%s' % (comment_txt, define_txt)
On 2011/08/24 20:49:11, dmichael wrote:
> Doesn't the % operator seem like overkill here?
> out += comment_txt + define_text
> 
> Or the whole thing could be:
> if comment_txt and comment:
>   out += comment_txt
> out += define_txt
> 
> Maybe this is a matter of preference, but it seems simpler to me to do the
> unconditional stuff unconditionally.

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:463: line = '%s%s' % (tab, line)
On 2011/08/24 20:49:11, dmichael wrote:
> again with the percent operator for just concatenation...  is this a
> Google/Python style thing I just haven't been indoctrinated in to yet? If it's
> the 'normal' way, then fine. Just seems strange to me.

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_generator.py
File ppapi/generators/idl_generator.py (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/idl_generator....
ppapi/generators/idl_generator.py:119: class GeneratorByFile(Generator):
On 2011/08/24 21:15:29, dmichael wrote:
> Could you add a brief comment for the class?
> 
> By the way, in the future, you should default to doing docstrings for class
and
> method documentation (as opposed to just #)

Done.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
File ppapi/generators/test_cgen_range/versions.h (right):

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:15: * File Comment. */
On 2011/08/24 21:15:29, dmichael wrote:
> Looks like a problem in generating that comment...  shouldn't the */ be on the
> next line?

This is correct based on the input IDL, but I can convert both to make it look
more like our common use case.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:22: /* Bogus Struct */
On 2011/08/24 21:15:29, dmichael wrote:
> Should the comment become a doxygen-style comment? Or did it have to be one in
> the idl file for that to happen?

Yes.  This is correct based on the input IDL.  The input IDL needs to be
formated for Doxygen, if you want to generate it.

http://codereview.chromium.org/7715036/diff/1/ppapi/generators/test_cgen_rang...
ppapi/generators/test_cgen_range/versions.h:35: int32_t (*Foo)(int32_t x,
int32_t y, int32_t z);
On 2011/08/24 21:15:29, dmichael wrote:
> Maybe you can throw a comment somewhere (wherever makes sense) that this is a
> work in progress...  as you know, the above code isn't right for Chrome or the
> public-facing headers yet. I don't want to ask you to fix all that in this CL,
> but it would be good to have a quick comment somewhere to that effect.

The place that makes sense is the CL itself I think.

[dmichael (off chromium), 2011-08-24 22:46:55]

LGTM to unblock you. I'll circle back and double check stuff on Monday.

[dmichael (off chromium), 2011-08-29 19:21:01]

On 2011/08/24 22:46:55, dmichael wrote:
> LGTM to unblock you. I'll circle back and double check stuff on Monday.
Circled back as promised; LGTM. Thanks for making those changes!