feat(codegen): add options to control indentation#12691
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
@copilot please continue |
I've implemented configurable indentation support for oxc codegen! The implementation includes: New Features:
Technical Changes:
Usage: // Tabs (default behavior)
let options = CodegenOptions::default();
// 2 spaces per indent
let options = CodegenOptions {
indent_style: IndentStyle::Spaces,
indent_size: 2,
..CodegenOptions::default()
};
// 4 spaces per indent
let options = CodegenOptions {
indent_style: IndentStyle::Spaces,
indent_size: 4,
..CodegenOptions::default()
};All existing tests pass, ensuring no regressions. The implementation is ready for use! Commit: bc39ec1 |
|
@overlookmotel do we need to modify |
I've refactored the implementation as suggested! The
This is much cleaner - the indentation logic is now centralized in |
|
@copilot |
Done! I've optimized the |
|
@copilot Please make Please rename |
|
@Boshen Can you please make the requests to copilot I made above? It doesn't seem to work if the comment isn't by you (is this a setting we can change?) |
|
I'll need to review the |
|
@copilot Please make IndentChar a #[repr(u8)] enum with the discriminants manually set to b' ' and b'\t'. Then you can remove indent_byte field. indent_char as u8 will play the same role. Please rename with_indent_config to with_indent. And same for any other places using "indent_config". "indent_config" is unnecessarily verbose. |
Done! I've implemented both requested optimizations:
The optimization maintains the same performance characteristics while eliminating redundant storage and simplifying the code. All tests pass. Commit: a47101c |
|
@copilot Please:
cc @Boshen. I wonder if it works if you just say "@copilot do what @overlookmotel said in previous comment". or even "@copilot watch for comments on this issue and do anything that @overlookmotel tells you to"! |
|
@copilot do what @overlookmotel said in previous comment |
Done! I've implemented all 8 requested changes:
The indentation system is now cleaner and more consistent. All tests pass and the new functionality works as expected. Commit: 9b64408
Completed all of @overlookmotel's requests! The indentation system has been fully refactored with cleaner APIs and enhanced functionality. All tests pass. Commit: 9b64408 |
overlookmotel
changed the title
github-actions
bot
added
A-codegen
overlookmotel
force-pushed
the
copilot/fix-12008568-45f0-406e-a8b2-665d1e5425af
branch
from
8c98740 to
f73f162
Compare
CodSpeed Instrumentation Performance ReportMerging #12691 will not alter performanceComparing Summary
|
|
Have rebased on main, run |
overlookmotel
deleted the
copilot/fix-12008568-45f0-406e-a8b2-665d1e5425af
branch
Add ability to control indentation style (tabs/spaces) and width (e.g. 2 / 4).
## Usage
```rust
use oxc_codegen::{Codegen, CodegenOptions, IndentChar};
// Tabs with default width
let options = CodegenOptions::default();
// Spaces with width 4
let options = CodegenOptions {
indent_char: IndentChar::Space,
indent_width: 4,
..CodegenOptions::default()
};
// Tabs with width 2
let options = CodegenOptions {
indent_char: IndentChar::Tab,
indent_width: 2,
..CodegenOptions::default()
};
```
All existing functionality is preserved with backward compatibility maintained. The updated system enables new use cases like multi-tab indentation.
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
Add ability to control indentation style (tabs/spaces) and width (e.g. 2 / 4).
## Usage
```rust
use oxc_codegen::{Codegen, CodegenOptions, IndentChar};
// Tabs with default width
let options = CodegenOptions::default();
// Spaces with width 4
let options = CodegenOptions {
indent_char: IndentChar::Space,
indent_width: 4,
..CodegenOptions::default()
};
// Tabs with width 2
let options = CodegenOptions {
indent_char: IndentChar::Tab,
indent_width: 2,
..CodegenOptions::default()
};
```
All existing functionality is preserved with backward compatibility maintained. The updated system enables new use cases like multi-tab indentation.
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
Add ability to control indentation style (tabs/spaces) and width (e.g. 2 / 4).
## Usage
```rust
use oxc_codegen::{Codegen, CodegenOptions, IndentChar};
// Tabs with default width
let options = CodegenOptions::default();
// Spaces with width 4
let options = CodegenOptions {
indent_char: IndentChar::Space,
indent_width: 4,
..CodegenOptions::default()
};
// Tabs with width 2
let options = CodegenOptions {
indent_char: IndentChar::Tab,
indent_width: 2,
..CodegenOptions::default()
};
```
All existing functionality is preserved with backward compatibility maintained. The updated system enables new use cases like multi-tab indentation.
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
#12691 added the option to write indentation as either tabs or spaces. This changed 2 things about writing indentation: 1. On x86_64, it now takes 3 SIMD instructions to "splat" the indent character across a 16-byte XMM register. Previously it was a 16-byte XMM read from a static. 2. If using spaces as indent, it'll usually be in width of 2 or 4. The indent printer has a fast path for writing less than 16 bytes of indentation. When using a single tab as indent, it's rare you'd write more than 16 bytes, because code is rarely that deeply nested. But if using 4 spaces as indent, depth only needs to get to 5 to exceed 16 bytes, which is not uncommon. This makes hitting the slow path far more likely. Therefore, increase the size of chunks of indentation that's written on the fast path to 32 bytes. This adds only one more XMM write, because it can reuse the "splatted" XMM register, and makes it less likely to hit the slow path. It's not possible to optimize for all cases because tabs and spaces indentation are quite different in this respect, but I think increasing the chunk size to 32 is probably a decent "middle of the road" option. Seems to have no ill effects on our benchmarks, which use tabs for indentation. So this seems not to hurt the tabs case, but should help the 4 x spaces case significantly.
oxc-project#12691 added the option to write indentation as either tabs or spaces. This changed 2 things about writing indentation: 1. On x86_64, it now takes 3 SIMD instructions to "splat" the indent character across a 16-byte XMM register. Previously it was a 16-byte XMM read from a static. 2. If using spaces as indent, it'll usually be in width of 2 or 4. The indent printer has a fast path for writing less than 16 bytes of indentation. When using a single tab as indent, it's rare you'd write more than 16 bytes, because code is rarely that deeply nested. But if using 4 spaces as indent, depth only needs to get to 5 to exceed 16 bytes, which is not uncommon. This makes hitting the slow path far more likely. Therefore, increase the size of chunks of indentation that's written on the fast path to 32 bytes. This adds only one more XMM write, because it can reuse the "splatted" XMM register, and makes it less likely to hit the slow path. It's not possible to optimize for all cases because tabs and spaces indentation are quite different in this respect, but I think increasing the chunk size to 32 is probably a decent "middle of the road" option. Seems to have no ill effects on our benchmarks, which use tabs for indentation. So this seems not to hurt the tabs case, but should help the 4 x spaces case significantly.
Add ability to control indentation style (tabs/spaces) and width (e.g. 2 / 4).
Usage
All existing functionality is preserved with backward compatibility maintained. The updated system enables new use cases like multi-tab indentation.