[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kdevelop-devel
Subject:    Re: new PHP Parser
From:       "Niko Sams" <niko.sams () gmail ! com>
Date:       2008-06-08 16:09:28
Message-ID: 629542d40806080909r4e0195f5x3f93b4f6f5113f93 () mail ! gmail ! com
[Download RAW message or body]

Hi,

Thanks for the first review - i'm glad you like it.

> /me looks forward to do Drupal development on KDevelop4
> with PHP autocompletion :)
yep - that would be cool!
(though still a long way to go)

>> PAAMAYIM_NEKUDOTAYIM ("::")
> Er, pretty strange name for a token?
It's Hebrew and used in PHP - see
http://en.wikipedia.org/wiki/Paamayim_Nekudotayim
A bit hard to work with (I copy&paste the name all the time)

>> IS_EQUAL ("=="), (...), EQUAL ("=")
> Might just be personal taste, but I think it would be good for readability to
> make those two a *bit* more easier to tell apart. Like, renaming the
> assignment operators from EQUAL to ASSIGN.
Changed as you suggested.

>>     parameter=functionCallParameterListElement @ COMMA | 0
>>  -> functionCallParameterList ;;
> (and similar rules)
>
> I found it a good idea to not have any rules that may be empty (except for
> pseudo data processing rules), as that makes stuff more predictable and
> slightly easier to debug. Most importantly, it prevents you from having the
> optional zero both in the caller rule and in the called rule.
you mean I should remove the "| 0" from the rule, for example:
#parameters=functionCallParameterListElement @ COMMA ->
functionCallParameterList ;;
LPAREN (parameterList=functionCallParameterList | 0) RPAREN -> foo;
instead of:
#parameters=functionCallParameterListElement @ COMMA | 0 ->
functionCallParameterList ;;
LPAREN parameterList=functionCallParameterList RPAREN -> foo;

> Also, "parameter" should be a list here ("#parameter") if I get that right.
fixed.

>>     ParserState _MState;
> Maybe I'm missing something, but what's the point of having those variables
> duplicated? I think you should either drop _MState or the two separate ones.
> Please do also care for consistency - if you keep _MState, it should probably
> be named m_state like the other member variables that you defined.
corrected.
(I do now understand how this works :D)

> All minor nitpicking, of course. I guess I could go on with pointless style
> issues all day, but let's instead just enjoy a fine new parser :D
I'm always happy about constructive criticism!


niko

_______________________________________________
KDevelop-devel mailing list
KDevelop-devel@kdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
[prev in list] [next in list] [prev in thread] [next in thread]