IfStatement's ToString()

Mar 5, 2009 at 10:25 PM
I'm using this parser for an academic project, and I've been really happy with it so far.  I'd like to upload a patch, but every checkout I do seems to have line ending issues (and I've tried in OSX, Windows, and Linux).  Rather than post a diff where every line has a changed line ending, I'll just suggest the change here.

I'm parsing source, performing some transformations on it, and then writing it back out again.  I've noticed that my if statement's always have else clauses, even if those statement blocks are empty.  Since I access the ElseStatements property when I am doing my transformation, it always gets created.  As a result, since the elseStatements member is not null, it always gets written out by ToString.

I'd like to change line 44 of IfStatement.cs to:
if ((elseStatements != null) && (elseStatements.Statements.Count > 0))
This is mostly a cosmetic issue, but it does improve readability of the output.  Let me know what you think.  I'm sorry that I can't create a decent patch, I don't know what is going on with the line endings.

David Thulson
Coordinator
Mar 5, 2009 at 11:34 PM
Hmm, I totally see your point, however I've also used ToSource a lot for debugging, and in those cases it might be useful to have that (eg for unit testing even, not that we have that, but someone might : ). Probably what we really need is formatting options there, so it can do everyone's preferred brace indent etc. Then you could specify not to write empty blocks out there.

For now it isn't a big change either way, so feel free to overrule me anyone : )
Developer
Mar 6, 2009 at 9:20 AM
David's patch has my vote. I'm a big supporter of readable generated code and empty blocks are just senseless for an else.
I think they do make sense for empty bodies of loop constructs, though, as a semicolon may be overlooked easily.

@David: I remember also having these issues. The problem is, that using SVN with CodePlex, it is (or was) not possible to correctly handle svn:eol-style native. And I think it also wasn't possible to remove the broken attribute again...
Coordinator
Mar 6, 2009 at 3:21 PM
Ok fair enough -- I can add that, or you, or David if you want to join and add it just let me know : )

I do sometimes use empty else's, but they would have a comment, which will get lost here anyway (if I have an 'else if' and all consitions aren't satisfied, I like showing I was aware of this by adding a default else - like and empty default in a switch. If the gen code added an empty else that would be wrong for sure, but in this case David's transformation specifically added one, so I was on the fence.
Mar 10, 2009 at 7:06 PM
I'd be interested to join and make the change, but maybe it would be best to wait a bit for me to get my feet wet a little more.  If I were you, I'm not sure I'd want some guy with a very incomplete understanding of the project mucking around in my repo!  If you want me to do it, that's fine.  I won't make any other commits without discussing it with someone first.  If you do want to wait a bit, I could still add it later since this really isn't a showstopper...

David Thulson