Expand list of files formatted by Verible #6031
Conversation
rkapuscik
force-pushed
the
rk/allow-list-formatted
branch
from
7760d48
to
b8c2c95
Compare
rkapuscik
requested review from
eunchan,
msfschaffner,
sjgitty,
sriyerg,
tjaychen and
weicaiyang
as code owners
There was a problem hiding this comment.
Thanks for making progress on the formatter front, @rkapuscik!
These changes look fairly uncontroversial to me.
|
|
||
|
|
There was a problem hiding this comment.
Could the formatter get rid of these extra unnecessary newlines?
There was a problem hiding this comment.
that would actually be a useful feature! @hzeller
also, quick question: can the formatter replace tabs and trim trailing whitespace?
we see that creeping into the code base all the time...
There was a problem hiding this comment.
IIRC, if there are more than two newlines, they are compacted to two (but still leaving the freedom to the developer to distringuish between one line and 'more space'). @rkapuscik can probably confirm.
(But we are also currently avoiding re-formatting too long lines into multiple lines, so could be that this feature might not be active at this point as it involves line-break handling).
TAB -> space and removing trailing newline is handled by the formtter, yes.
| @@ -55,7 +55,7 @@ module adc_ctrl ( | |||
| .reg2hw(reg2hw), | |||
| .hw2reg(hw2reg), | |||
| .intg_err_o(), | |||
| .devmode_i (1'b1) | |||
| .devmode_i(1'b1) | |||
There was a problem hiding this comment.
Can the formatter align the brackets on these lines (51-58)?
There was a problem hiding this comment.
Seems like the formatter is sometimes aligning brackets, but not in this case. Antmicro is looking into it.
There was a problem hiding this comment.
This is controlled by --named_port_alignment option in Verible. By default it is set to infer (it was not explicitly set in the original formatter script), but if the consensus is to have named port connections aligned we can set it to align.
There was a problem hiding this comment.
Thanks for checking that @rkapuscik. I opened lowRISC/style-guides#48 to get indication on which style we prefer, but for this PR the change is fine as-is: it keeps the existing style for this particular instantiation (which is non-tabbed), and just applies this rule consistently.
There was a problem hiding this comment.
Nice work, thanks! This PR will need one rebase to make the merge conflict go away and then we're ready to go!
I'll ask people to jump in on lowRISC/style-guides#48 to potentially find a project-wide consensus on the preferred formatting.
Signed-off-by: Rafal Kapuscik <[email protected]>
rkapuscik
force-pushed
the
rk/allow-list-formatted
branch
from
b8c2c95
to
9a43e5f
Compare
This PR is split from #6027 to keep the formatting changes separate.
It expands the Verible formatter allow list with some files where the changes were reviewed and found compliant to the lowRISC style guide.