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 verilog code with verible-verilog-format #280

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

jthornblad
Copy link
Contributor

Description

Format verilog code with verible-verilog-format.

Used flags:
--indentation_spaces=2
--wrap_end_else_clauses=true

Type of change

  • Bugfix (non breaking change which resolve an issue)
  • Feature (non breaking change which adds functionality)
  • Breaking Change (a change which would cause existing functionality to not work as expected)
  • Documentation (a change to documentation)

Submission checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my changes
  • I have tested and verified my changes on target
  • My changes are well written and CI is passing
  • I have squashed my work to relevant commits and rebased on main for linear history
  • I have added a "Co-authored-by: x" if several people contributed, either pair programming or by squashing commits from different authors.
  • I have updated the documentation where relevant (readme, dev.tillitis.se etc.)
  • QEMU is updated to reflect changes

@dehanj
Copy link
Member

dehanj commented Oct 18, 2024

I rebased this on main and pushed.

In general I'm fine with the format, since the entire CI and especially the "check-hashes" job goes green it means that this does not affect the binary generated - which is great.

However, I miss two make targets to easily perform a format.
In other projects we use something like this

.PHONY: fmt
fmt:
	clang-format --dry-run --ferror-limit=0 $(FMTFILES)
	clang-format --verbose -i $(FMTFILES)

.PHONY: checkfmt
checkfmt:
	clang-format --dry-run --ferror-limit=0 --Werror $(FMTFILES)

One target to check (checkfmt above) if the code is formatted or not, and fails if it isn't. That can be run in CI and prevent us from merge unformatted code by mistake.

The other target (fmt above) does a format and writes it to the files needed. It also makes it easier since I don't have to figure out what Verible command you ran to achieve this format.

I have not looked into verible, does it need a similar config file such as ".clang-format" that we should include? Or is it simply a command?

If you add the make targets, at least the one that formats and the other to check the format if it is possible I'm happy enough to approve.

@jthornblad
Copy link
Contributor Author

...

One target to check (checkfmt above) if the code is formatted or not, and fails if it isn't. That can be run in CI and prevent us from merge unformatted code by mistake.

The other target (fmt above) does a format and writes it to the files needed. It also makes it easier since I don't have to figure out what Verible command you ran to achieve this format.

Added both.

I have not looked into verible, does it need a similar config file such as ".clang-format" that we should include? Or is it simply a command?

It's a command, no config files for formatting what I know.

@dehanj dehanj force-pushed the format_verilog_code branch 2 times, most recently from f9a6a50 to d7ab694 Compare October 22, 2024 09:58
Flags:
        --indentation_spaces=2
        --wrap_end_else_clauses=true

Verify flag, used in checkfmt, only returns error if the last file is
not formatted, temporary fix implemented with grep.
@dehanj
Copy link
Member

dehanj commented Oct 22, 2024

Split the commit into two commits, added short description on why grep is used in checkfmt.

Tested it on target. Check-hashes shows the output is the same.
Approved.

This can be added to CI as soon as the new tkey-builder is deployed.

@dehanj dehanj merged commit 3514d7e into main Oct 22, 2024
5 checks passed
@dehanj dehanj deleted the format_verilog_code branch October 22, 2024 10:11
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.

2 participants