go: make package comment more ergonomic - #392
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #392 +/- ##
==========================================
+ Coverage 96.53% 97.10% +0.56%
==========================================
Files 10 14 +4
Lines 750 932 +182
==========================================
+ Hits 724 905 +181
- Misses 26 27 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Go use explict extension, as extensions grows
it's harder for developer to add extentions without change the
whole generated line.
This PR generate right aligned comments and split line by four
sorted extensions
i.e.
```
make inst.go EXTENSIONS='\
rv64_a rv64_c rv64_d rv64_f \
rv64_i rv64_m rv64_q rv64_zba \
rv64_zbb rv64_zbs rv_a rv_c \
rv_c_d rv_d rv_f rv_i \
rv_m rv_q rv_s rv_system \
rv_v rv_zba rv_zbb rv_zbs \
rv_zicond rv_zicsr \
'
```
So that developers can add more extensions by copy/paste on terminal
without pain.
Also change the original `python parse.py` command line into `make`
9094672 to
727b88a
Compare
|
Meng Zhuo (@mengzhuo) while I agree that it is getting slightly unwieldy, I'm not convinced that this is substantially better. I have to wonder if listing the extensions (one per line) in a separate file (possibly even added to the Go repo) might be preferable... that said, it's also not that difficult to put the existing list into a file, edit and sort it, then feed it in to the current tooling. |
Jay Dev Jha (IIITM-Jay)
left a comment
Jay Dev Jha (IIITM-Jay)
left a comment
There was a problem hiding this comment.
Meng Zhuo (@mengzhuo) Thanks for the PR!
I have left some comments which I felt to be addressed. Feel free to keep your opinions as well.
| n = 4 | ||
| exts = sorted(extensions) | ||
| for i in range(0, len(exts), 4): | ||
| ext_line = " ".join(exts[i : i + n]) |
There was a problem hiding this comment.
Meng Zhuo (@mengzhuo) , This is a bit misleading. The piece of code will end up in breaking the logic intended. The chunk size is controlled by n and the iteration is hardcoded as 4 and again inner logic still depends on n. If someone later changes n to 8 (thinking it controls grouping), the code breaks because, outer loop still advances by 4, but inner slice chooses 8. Consequently, creating duplicated extensions and overlapping groups, leading to generating incorrect outputs.
| exts = sorted(extensions) | ||
| for i in range(0, len(exts), 4): | ||
| ext_line = " ".join(exts[i : i + n]) | ||
| comment += f"{ext_line:>30} \\\n" |
There was a problem hiding this comment.
IMO, it would be better to have adjusting logic to support variable-length, instead of making it as a hardcoded limit
| for i in range(0, len(exts), 4): | ||
| ext_line = " ".join(exts[i : i + n]) | ||
| comment += f"{ext_line:>30} \\\n" | ||
| comment += "'\n*/\n" |
There was a problem hiding this comment.
If extensions is empty the code will behave adversely causing the generated Go file to treat everything as inside the comment block. The code must handle the exception cases as well.
| if extensions: | ||
| comment += " EXTENSIONS='\\\n" | ||
| n = 4 | ||
| exts = sorted(extensions) |
There was a problem hiding this comment.
I am not sure with the sorting of extension here as if suppose a tool relies on a particular order (for readability, or minimal diff when adding one extension), this change may cause larger diffs than necessary each time a new extension is added.
| /* | ||
| make inst.go""" | ||
| if extensions: | ||
| comment += " EXTENSIONS='\\\n" |
There was a problem hiding this comment.
As these are going to be inside comment /*...*/, I believe that the backslashes have no effect on Go compilation.
3 participants
Go use explict extension, as extensions grows
it's harder for developer to add extentions without change the whole generated line.
This PR generate right aligned comments and split lines by four sorted extensions
i.e.
So that developers can add more extensions by copy/paste on terminal without pain.