On 2013-11-19 11:32, Jonathan M Davis wrote:
That looks pretty good overall, though there are probably a few places where I'd favor having fewer newlines. e.g. the body of the foreach in translateFunction is formatted similarly to what I'd do by default, but the overall feel is that it has too much vertical space. So, I might be inclined to remove some of the newlines, much as I'd want them by default. The fact that you have braces on the first version block doesn't help either. But the formatting is okay.
That particular version block is from a pull request and doesn't follow the overall formatting. I wasn't so picky about the formatting in this case.
While I suspect that Andrei would definitely want to remove many of those newlines if he were formatting the code, what I think Andrei was really objecting to in your comments said that you wanted to put a newline after _every_ statement (which you're not actually doing in your code), and then you wanted separate lines between "paragraphs of code," implying that you'd end up with two newlines separating out sections of your functions. And that would be a lot of extraneous newlines. What you actually seem to have is putting a newline around "paragraphs" but not a newline between every statement, which is a lot more reasonable (though if your "paragraphs" are too small, you run into the exact problem that Andrei was commenting on in my code that started this discussion).
Looking at code in Phobos I would suspect Andrei wanting to remove all empty newlines inside functions.
No, I don't want two empty newlines next to each other. I guess I didn't phrase my self very clearly. Your explanation of putting a newline around "paragraphs" seems better.
By the way, you have a lot of useless breaks in your code. A break isn't actually required if there's a return statement or other control statement such as continue at the end of the case statement. Maybe it's a stylistic thing? But in case you didn't know, they're completely unnecessary, and personally, I'd consider them to be clutter. It's your code though, not mine.
Hmm, I don't remember how I ended up with that. I do know that they're not required. Perhaps it was a stylistic choice. I think it groups things nicely, just as braces do.
-- /Jacob Carlborg
