Closed Bug 1472101 Opened 3 years ago Closed 3 years ago

[BinAST] Fix some styling in BinAST auto-generated code.

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(3 files)

Before updating the BinAST code to match the latest spec, I'd like to apply some style fix.
Just to make it possible to update auto-generated files easily, added a script to do cargo run.
Attachment #8988668 - Flags: review?(dteller)
Instead of always put newline, added WrapLine trait with wrap_line function,
which appends newline if the given string is not empty.
so that we won't have duplicate empty lines.
Attachment #8988669 - Flags: review?(dteller)
Just added a missing space.
Attachment #8988670 - Flags: review?(dteller)
Summary: Fix some styling in BinAST auto-generated code. → [BinAST] Fix some styling in BinAST auto-generated code.
Depends on: 1472103
Attachment #8988668 - Flags: review?(dteller) → review+
Comment on attachment 8988669 [details] [diff] [review]
Part 1: Remove some duplicate empty lines in auto-generated BinAST code.

Review of attachment 8988669 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. Just some tiny nitpicking :)

::: js/src/frontend/binsource/src/main.rs
@@ +19,5 @@
>  
>  use itertools::Itertools;
>  
> +/// A string or string-like construction that can be wrapped.
> +trait WrapLine {

Is that really "wrapping"?

Also, I suspect that you should rather patch `ToStr::newline` in binjs_meta/src/util.rs, no?
Attachment #8988669 - Flags: review?(dteller) → review+
Attachment #8988670 - Flags: review?(dteller) → review+
Thank you for reviewing :D

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)
> > +/// A string or string-like construction that can be wrapped.
> > +trait WrapLine {
> 
> Is that really "wrapping"?

Yeah, newline or newline_if_SOMETHING would be better.


> Also, I suspect that you should rather patch `ToStr::newline` in
> binjs_meta/src/util.rs, no?

I'll patch it later, and then switch to use it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcfc8b1be986b3b7c5d86e0514ab0b16df5d607
Bug 1472101 - Part 0: Add a script to build auto-generated BinAST code. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7abf84687b599ec3168f4aac4bb8087eb9f620
Bug 1472101 - Part 1: Remove some duplicate empty lines in auto-generated BinAST code. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb57020766137aa6f654fdc38fd2f96b9181dd01
Bug 1472101 - Part 2: Add a space after switch in auto-generated BinAST code. r=Yoric
https://hg.mozilla.org/mozilla-central/rev/1fcfc8b1be98
https://hg.mozilla.org/mozilla-central/rev/fc7abf84687b
https://hg.mozilla.org/mozilla-central/rev/fb5702076613
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1472103
No longer depends on: 1472103
You need to log in before you can comment on or make changes to this bug.