Generate better formatted Rust code#8919
Conversation
|
@jtdavis777 it looks like you've been reviewing recent PRs for this project. I have a few more Rust style cleanups I would love to do. They aren't functional changes, so should be pretty straightforward to review. Can I send them your way? |
|
Sure I can add this to the docket. I am not a rust dev at all but am happy to look this over. |
|
Great, thank you! |
jtdavis777
left a comment
There was a problem hiding this comment.
Overall the changes to the generator code look good, I have two questions
| code_ += " }"; | ||
| code_ += " }"; | ||
| code_ += "}"; | ||
| code_ += ""; |
There was a problem hiding this comment.
curious why you have a function to do this, but also a bunch of new lines that don't use the function?
There was a problem hiding this comment.
The idea is sort of that within each generator function (GenStruct, GenTable, etc), there's little conditional behavior, so after each block is emitted we can also emit a newline to leave a gap between the end of this item and the beginning of the next. And then just not emit a newline for the very last item.
With the top level items (GenStruct, GenTable, etc), there was a little bit of conditional behavior around whether we needed to emit a newline or not, which was based on if we had entered a new mod or not (sort of like a namespace block in C++). After #8877, this became not true, because there were always use statements at the top of the mod scope, so we could unconditionally just add a newline and get perfect formatting.
When I rebased this change on #8877, I basically just dropped the conditional part from AddLineBeforeContentIfNeeded. You're right that it's confusingly named now. I've updated the name, and added some documentation around how it's expected to be used.
I think it'd be reasonable to keep this function around for the Gen* functions for the purpose of documentation, and incase we do need to bring back conditional behavior in the future. But I'm also not attached to it. Happy to just remove the function and bulk replace with code_ += "";. I don't really want to replace every instance of code_ += "" with the function, because I think it obfuscates more than it adds.
Let me know what you'd prefer to do here. I'm pretty flexible!
There was a problem hiding this comment.
I love the idea but think for the sake of maintenance having the consistency of explicitly adding the newline is better and removing the function. Thanks for the detailed explanation!
There are no behavioral changes in the PR. This simply updates the Rust generator to produce better formatted code, with the following changes:
modblocks is properly indented.#[inline]annotations have been moved below function documentation, which is the expected style.The first commit contains the relevant changes.
The second commit simply regenerates all the example code.
It would be easiest to review the commits separately.