Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format chosen files using Verible formatter #6027

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

rkapuscik
Copy link
Contributor

This PR introduces changes to Verible formatter script allowing it to format only a subset of OpenTitan files, along with a starting list of such files and relevant formatting fixes.

List of starting files was created by running Verible formatter on the project and adding ones that either needed no formatting or the changes were reviewed and found compliant to the lowRISC style guide. It is not complete in order to keep review manageable, but can be extended easily.

cc @GregAC, @imphil, @hzeller

@rswarbrick
Copy link
Contributor

This looks like it might be a bit complicated to get merged, but most of the lint-fix changes look pretty uncontroversial to me. Maybe it would be worth splitting them into separate PRs? Then the "turn on the tool so I don't have to do things manually any more" PR ends up much smaller (and won't bit-rot quite as quickly).

@GregAC
Copy link
Contributor

GregAC commented Apr 9, 2021

Thanks for this @rkapuscik

Given the simplicity of the formatting changes I'd hope we could do this in one or two PRs (one for format changes the other for the scripting).

I'd suggest squashing the commits down. Having all the formatting changes in a single commit is probably justifiable as they're all simple things and related in that they're all lint fixes to enable tooling. At least changes for related files should be squash together (no 'add changes', 'more changes' and 'final changes').

@rkapuscik
Copy link
Contributor Author

Thank you for your input, I moved the formatting changes to #6031 (to be updated after this PR is resolved) and kept here only files that were not affected by the Verible formatter.

@tgorochowik
Copy link
Contributor

@GregAC @imphil is this acceptable like this? Is there something we can do to proceed?

Copy link
Contributor

@imphil imphil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I left some smallish comments, can you get those resolved and we can then merge it?


case $MODE in
allowlist)
FILES_TO_FORMAT=`cat util/verible-format-allowlist.vbl | sed '/^#/d'`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FILES_TO_FORMAT=`cat util/verible-format-allowlist.vbl | sed '/^#/d'`
FILES_TO_FORMAT=`grep -v '^#' util/verible-format-allowlist.vbl`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cleaner, thank you. Changed.

@@ -0,0 +1,18 @@
# Copyright lowRISC contributors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a file with a .txt suffix or so, .vbl is for Verible waiver/config files which are consumed by Verible directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to .txt.

# This is a list of files to be passed to Verible formatter
# by calling verible-format.sh

./hw/ip/adc_ctrl/dv/tests/adc_ctrl_test_pkg.sv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for the ./

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

esac

echo $FILES_TO_FORMAT | \
xargs -n 1 -P $NUM_PROCS verible-verilog-format \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xargs -n 1 -P $NUM_PROCS verible-verilog-format \
xargs -n 1 -P $NUM_PROCS verible-verilog-format \

or align with the backslash above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

@imphil imphil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Would you mind combining the two commits into one, and then I'll merge it?

For now it contains only files unchanged by the formatter.

Signed-off-by: Rafal Kapuscik <[email protected]>
@rkapuscik
Copy link
Contributor Author

No problem, I combined them.

@imphil imphil merged commit 139cbb9 into lowRISC:master Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants