Add IDL Lexer

Add IDL Lexer


This lexer understands standard IDL tokens which are similar to 'C'.
INT, HEX, FLOAT, QUOTE and SYMBOL.   SYMBOL can then also become a
KEYWORD such as enum, interface, struct, typedef...

R=ncbray@google.com
BUG=76237
TEST=python idl_lexer.py --test_expect --test_same test_lex.in

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79169

[Nick Bray, 2011-03-15 21:44:06]

A quick mechanical review.

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

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:22: except:
Don't bother?  Just modify the path, then import?  If the modified path is
bogus, it will just be a NOP.

Also, I've started a "pylib" directory in third_party to put python modules
in... but I am not sure if this is a good idea, because Chrome imports directly
from third_party in some cases.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:27: from ply import yacc
Unused?

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:36: class IDLLexer(object):
Please comment the lex magic.

There are lots of statements that appear to be unused, at first glance, if you
aren't expecting the magic.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:75: t_FLOAT =
r'-?(\d+\.\d*|\d*\.\d+)([Ee][+-]?\d+)?|\d+[Ee][+-]?\d+'
FYI: negative literal numbers can be a pain if you support binary expressions. 
I think you're safe, however.

I think -1e1 might be rejected, btw, because the second option does not allow
negative signs?

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:81: r'"[^"]*"'
Wrong for strings with an escaped double quote.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:82: t.value = t.value[1:-1]
Does not unescape characters.

Consider eval(t.value)?

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:83: self.AddLines(t.value.count('\n'))
Count before converting the string?  (\\n -> \n when converted)

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:88: r'(/\*(?s).*?\*/)|((?m)//.*?$)'
The (?m) option seems a little weird.  Why?

Did you mix up the (?s) and the (?m)?

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:93: def t_KEYWORD_SYBOL(self, t):
Typo

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:94: r'[A-Za-z][A-Za-z_0-9]*'
Leading underscores?

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:105: print >>sys.stderr, out
Prefer:
sys.stderr.write(out+'\n')

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:110: self.index.append(self.lexobj.lexpos)
This seems a little odd in cases where count > 1...

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:141: def FilesToTokens(filenames, verbose =
False):
no spaces around default argument.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:147: if verbose:print '\tLoaded %s...' % filenames
space or stick on new line.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:177: def Test(tokens, output = False, verbose =
False):
No spaces

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:199: opts, filenames = getopt.getopt(args, '',
long_opts)
I prefer optparse.  Consider build/download_chrome.py.

It doesn't require that you plumb the arguments through, etc.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:202: print >>sys.stderr, 'Illegal option:', str(e)
Again, >> syntax is nonstandard.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:210: for opt, val in opts:
optparse would do away with this silliness.

[noelallen1, 2011-03-17 01:20:02]

PTAL

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

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:22: except:
I try instead of forcing it, we do know now if it's already in the path.  I
figured the try was cleaner than attempting to figure out if it's already
available.

On 2011/03/15 21:44:06, Nick Bray wrote:
> Don't bother?  Just modify the path, then import?  If the modified path is
> bogus, it will just be a NOP.
> 
> Also, I've started a "pylib" directory in third_party to put python modules
> in... but I am not sure if this is a good idea, because Chrome imports
directly
> from third_party in some cases.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:27: from ply import yacc
On 2011/03/15 21:44:06, Nick Bray wrote:
> Unused?

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:36: class IDLLexer(object):
On 2011/03/15 21:44:06, Nick Bray wrote:
> Please comment the lex magic.
> 
> There are lots of statements that appear to be unused, at first glance, if you
> aren't expecting the magic.

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:75: t_FLOAT =
r'-?(\d+\.\d*|\d*\.\d+)([Ee][+-]?\d+)?|\d+[Ee][+-]?\d+'
On 2011/03/15 21:44:06, Nick Bray wrote:
> FYI: negative literal numbers can be a pain if you support binary expressions.

> I think you're safe, however.
> 
> I think -1e1 might be rejected, btw, because the second option does not allow
> negative signs?

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:81: r'"[^"]*"'
We do not allow escaped quotes.
On 2011/03/15 21:44:06, Nick Bray wrote:
> Wrong for strings with an escaped double quote.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:82: t.value = t.value[1:-1]
On 2011/03/15 21:44:06, Nick Bray wrote:
> Does not unescape characters.
> 
> Consider eval(t.value)?

Same as above.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:83: self.AddLines(t.value.count('\n'))
On 2011/03/15 21:44:06, Nick Bray wrote:
> Count before converting the string?  (\\n -> \n when converted)

see above

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:88: r'(/\*(?s).*?\*/)|((?m)//.*?$)'
On 2011/03/15 21:44:06, Nick Bray wrote:
> The (?m) option seems a little weird.  Why?
> 
> Did you mix up the (?s) and the (?m)?

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:93: def t_KEYWORD_SYBOL(self, t):
On 2011/03/15 21:44:06, Nick Bray wrote:
> Typo

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:94: r'[A-Za-z][A-Za-z_0-9]*'
On 2011/03/15 21:44:06, Nick Bray wrote:
> Leading underscores?

Not allowed

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:105: print >>sys.stderr, out
On 2011/03/15 21:44:06, Nick Bray wrote:
> Prefer:
> sys.stderr.write(out+'\n')

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:110: self.index.append(self.lexobj.lexpos)
On 2011/03/15 21:44:06, Nick Bray wrote:
> This seems a little odd in cases where count > 1...

Commented.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:141: def FilesToTokens(filenames, verbose =
False):
On 2011/03/15 21:44:06, Nick Bray wrote:
> no spaces around default argument.

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:147: if verbose:print '\tLoaded %s...' % filenames
On 2011/03/15 21:44:06, Nick Bray wrote:
> space or stick on new line.

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:177: def Test(tokens, output = False, verbose =
False):
On 2011/03/15 21:44:06, Nick Bray wrote:
> No spaces

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:199: opts, filenames = getopt.getopt(args, '',
long_opts)
On 2011/03/15 21:44:06, Nick Bray wrote:
> I prefer optparse.  Consider build/download_chrome.py.
> 
> It doesn't require that you plumb the arguments through, etc.
see below

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:202: print >>sys.stderr, 'Illegal option:', str(e)
On 2011/03/15 21:44:06, Nick Bray wrote:
> Again, >> syntax is nonstandard.

Done.

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:210: for opt, val in opts:
On 2011/03/15 21:44:06, Nick Bray wrote:
> optparse would do away with this silliness.

optparse might be a reasonable change, but since this is already here, I'd
rather get that in as a separate CL than delay this.

[Nick Bray, 2011-03-21 20:25:29]

Nothing wildly objectionable.  (But I'll need to review everything together, at
the end.)

We can move on to the next file.

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

http://codereview.chromium.org/6697028/diff/1/ppapi/generators/idl_lexer.py#n...
ppapi/generators/idl_lexer.py:22: except:
You're trading one ugly for another.  I personally don't think you're gaining
anything, however.

On 2011/03/17 01:20:02, noelallen1 wrote:
> I try instead of forcing it, we do know now if it's already in the path.  I
> figured the try was cleaner than attempting to figure out if it's already
> available.
> 
> On 2011/03/15 21:44:06, Nick Bray wrote:
> > Don't bother?  Just modify the path, then import?  If the modified path is
> > bogus, it will just be a NOP.
> > 
> > Also, I've started a "pylib" directory in third_party to put python modules
> > in... but I am not sure if this is a good idea, because Chrome imports
> directly
> > from third_party in some cases.
>

http://codereview.chromium.org/6697028/diff/5003/ppapi/generators/idl_lexer.p...
ppapi/generators/idl_lexer.py:61: 'readonly' : 'READONLY',
I believe the style guide says:
'typedef': 'TYPEDEF'
(remove a space)

http://codereview.chromium.org/6697028/diff/5003/ppapi/generators/idl_lexer.p...
ppapi/generators/idl_lexer.py:158: #
Implement this in terms of TextToTokens

[Nick Bray, 2011-03-23 00:27:00]

LGTM with the understanding the test goes live ASAP.