-
Notifications
You must be signed in to change notification settings - Fork 1
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
Verible Format experiment #7
Conversation
bfce4ae
to
6795667
Compare
Signed-off-by: Michael Schaffner <[email protected]>
6795667
to
2bf9354
Compare
// allows the processor to go into sleep | ||
// before AES starts operation. | ||
module aes | ||
import aes_pkg::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism @hzeller @udinator @scottdj
do we expect imports to be indented or not?
looks like we tend to indent this in our codebase, e.g.:
https://github.com/lowRISC/opentitan/blob/914e1b21982805f11da55e8973e32fa6c67719ae/hw/ip/alert_handler/rtl/alert_handler.sv#L13-L19
https://github.com/lowRISC/opentitan/blob/914e1b21982805f11da55e8973e32fa6c67719ae/hw/ip/otbn/rtl/otbn.sv#L10-L18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that they are not handled at this time and will need a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner we have done a similar exercise in lowRISC/ibex#1014 for Ibex, which is a much smaller code base and easier to get some of the things sorted out. It would probably be helpful to get Ibex done first, and then look at OpenTitan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great, thanks for the pointer!
Note that we are at this time not specifically working on getting this merged into upstream. This is just a dummy PR to generate some feedback on various aspects of the code base, including DV code.
I fully agree that trying to get mass-formatting to work well on Ibex is probably a better intermediate milestone.
Btwy, @fangism just told me that there are a couple of upcoming improvements related to how Verible decides whether to format pre-aligned code or not (https://github.com/google/verible#aligned-formatting). Give it another day or two such that these patches can be rolled out as part of the GH releases, but then I guess it would be worth giving this another try on Ibex.
Also, Verible should get an experimental flag for OT-specific style differences (like 2/4 space differences) in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 vs. 4 space issue is tracked as chipsalliance/verible#40.
.prng_data_ack_i ( prng_data_ack ), | ||
.prng_data_i ( prng_data ), | ||
.prng_reseed_req_o ( prng_reseed_req ), | ||
.prng_reseed_ack_i ( prng_reseed_ack ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming that right now we do not align close-parentheses.
b = q ^ aes_square_scale_gf2p4_gf2p2(a1 ^ a0) | ||
^ aes_square_scale_gf2p4_gf2p2(m1 ^ m0) | ||
^ aes_mul_gf2p4(a1, a0) | ||
^ mul_a1_m0 ^ mul_a0_m1 ^ mul_m0_m1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism if we wanted to preserve such structures, we would have to add a //
comment to the end of the line, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just enabled more re-wrap disabling today for cases like this. Give a little time for auto-releases to catch up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, will check again later.
.COV_T(alert_handler_env_cov) | ||
); | ||
class alert_handler_virtual_sequencer extends cip_base_virtual_sequencer#( | ||
.CFG_T(alert_handler_env_cfg), .COV_T(alert_handler_env_cov) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this should be one connection per line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this "named parameters in parameterized type references".
This was just enabled for module instances earlier today, I'll test whether or not it works in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work yet, so filed chipsalliance/verible#428
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fangism!
[1 :1000] :/ 5, // reset during alert | ||
[1001 :16_500_000] :/ 2, | ||
[16_500_001 :17_000_000] :/ 2, // reset during ping | ||
[17_000_001 :18_000_000] :/ 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//coments
force line breaks, but do not cause anything else around them to be preserved.
FYI, alignment of dists is chipsalliance/verible#403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it, thanks!
accu_q; | ||
assign accu_d = (clr_i) ? '0 : // clear | ||
(trig_gated && !(&accu_q)) ? accu_q + 1'b1 : // saturate counter at maximum | ||
accu_q; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worse than that, even hang/wrap indentation doesn't look right.
Please file an issue with a reduced test case for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jep, tracked here: chipsalliance/verible#431
// the counter is zero in this state. so if the | ||
// timeout count is zero (==disabled), cnt_ge will be true. | ||
// the counter is zero in this state. so if the | ||
// timeout count is zero (==disabled), cnt_ge will be true. | ||
end else if (timeout_en_i && !cnt_ge && en_i) begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds difficult, but I see your point. Comment placement is a mysterious art. How bad would it be if that comment were moved inside the following else-if-begin block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha yeah I see the "mysterious art aspect". Moving the comment into the next block would be ok, just wanted to flag this since I think such comments before if/else conditions do occur once in a while...
state_q == Idle) | ||
`ASSERT(CheckTimeout0, | ||
state_q == Idle && timeout_en_i && en_i && timeout_cyc_i != 0 && !accum_trig_i |=> | ||
state_q == Timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit more here? To me it looks like a long expression, whose wrapping should beget +4 spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more expecting something like the example below:
`ASSERT(CheckTimeout0,
state_q == Idle && timeout_en_i && en_i && timeout_cyc_i != 0 && !accum_trig_i |=>
state_q == Timeout)
But by wrapping it like that I basically treat the whole macro as one single expression, which may be wrong in the first place...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening is that the wrapped argument is treated as one unit of formatting, whose standard rule is to hang-indent/wrap +4 spaces on continued lines. To to otherwise would require treating it as a hierarchy of units (of equal indentation). Might not get around to this any time soon.
@@ -5,22 +5,12 @@ | |||
module entropy_src_bind; | |||
|
|||
bind entropy_src tlul_assert #( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As coded, they do (behave same as data declarations). What appears wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verible reformats
bind entropy_src tlul_assert #(
.EndpointType("Device")
) tlul_assert_device (
.clk_i,
.rst_ni,
.h2d (tl_i),
.d2h (tl_o)
);
into
bind entropy_src tlul_assert #(
.EndpointType("Device")
) tlul_assert_device (.clk_i, .rst_ni, .h2d(tl_i), .d2h(tl_o));
which is a weird choice. I.e., it prefers one parameter assignment per line, and then puts all port connections onto a single line. I would expect it to leave the port connections one per line also...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does look surprising to me. Can you file an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think ideally we want bind
statements to look the same as a module/interface instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked here: chipsalliance/verible#432
m_eflash_tl_agent.monitor.a_chan_port.connect(scoreboard.eflash_tl_a_chan_fifo.analysis_export | ||
); | ||
m_eflash_tl_agent.monitor.d_chan_port.connect(scoreboard.eflash_tl_d_chan_fifo.analysis_export | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - I think I'd prefer to have it wrapped as it is in the 'before' part of the diff:
m_eflash_tl_agent.monitor.d_chan_port.connect(
scoreboard.eflash...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines that need wrapping should now be left intact by a recently released change. let's verify that in the next round.
input bit [5:0] mode_i, // 6'b00_0001 = ECB, 6'00_b0010 = CBC, 6'b00_0100 = CFB, | ||
// 6'b00_1000 = OFB, 6'b01_0000 = CTR, 6'b10_0000 = NONE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism this is a tricky one...both sets of comments are meant to apply to the input bit [5:0] mode_i
on line 12 - with the reformatting the comment on line 13 now behaves like it applies to the following lines of code (as per the style guide).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Would it then be wise to move the comments to before line 12 to follow style guidance? (whenever the trailing comment doesn't fit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think that is preferred by our style guide also (comments on a line with code describe that line of code only).
// 6'b00_1000 = OFB, 6'b01_0000 = CTR, 6'b10_0000 = NONE | ||
input bit [3:0][31:0] iv_i, | ||
input bit [2:0] key_len_i, // 3'b001 = 128b, 3'b010 = 192b, 3'b100 = 256b | ||
input bit [7:0][31:0] key_i, input bit [7:0] data_i[], output bit [7:0] data_o[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism I think we want to keep the original formatting instead of collapsing multiple input/output
s onto one line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2,7 +2,9 @@ | |||
// Licensed under the Apache License, Version 2.0, see LICENSE for details. | |||
// SPDX-License-Identifier: Apache-2.0 | |||
|
|||
class aes_env_cfg extends cip_base_env_cfg #(.RAL_T(aes_reg_block)); | |||
class aes_env_cfg extends cip_base_env_cfg#( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner I think we want to keep the space before the #
as per the Parameterized Types section, correct?
@fangism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would also expect a space before the #
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed there should be a space before #
. Could you file a new issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracked here: chipsalliance/verible#444
str = { | ||
str, | ||
$sformatf("\n\t ----| Reference model:\t %s \t ", | ||
(ref_model == 0) ? "C-MODEL" : "OPEN_SSL") | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism What if this was reformatted as shown below?
str = {str, $sformatf("\n\t ----| Reference model... \t ",
(ref_model == 0) ? ...)};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a possibility but it may be a while before we can make that happen. The low-level wrapping optimizer needs to be completely rewritten to evaluate rectangles and bounding boxes first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood - it shouldn't be that much of an issue imo.
if (fixed_keylen_en) {aes_keylen == fixed_keylen}; | ||
} | ||
|
||
constraint c_key { | ||
if (fixed_key_en) { | ||
aes_key[0] == fixed_key[0], | ||
aes_key[1] == fixed_key[1] | ||
}; | ||
} | ||
constraint c_key {if (fixed_key_en) {aes_key[0] == fixed_key[0], aes_key[1] == fixed_key[1]};} | ||
|
||
constraint c_iv { | ||
if (fixed_iv_en) { | ||
aes_iv == fixed_iv | ||
}; | ||
} | ||
constraint c_iv {if (fixed_iv_en) {aes_iv == fixed_iv};} | ||
|
||
constraint c_operation { | ||
if (fixed_operation_en) { | ||
aes_operation == fixed_operation | ||
}; | ||
} | ||
constraint c_operation {if (fixed_operation_en) {aes_operation == fixed_operation};} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism I think these constraints should be kept as the original formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, can you file a new issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if(input_item.data_in_valid() && input_item.key_clean(1) && input_item.iv_clean(1)) begin | ||
if (input_item.data_in_valid() && input_item.key_clean(1) && input_item.iv_clean(1) | ||
) begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism I think this is another unfortunate scenario where the closing parens just exceeds line length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next iteration, this will be left alone.
`uvm_info(`gfn, $sformatf("\n\t ----| Finish test received adding message item to mg_fifo") | ||
, UVM_MEDIUM) | ||
wait(finish_message) | ||
`uvm_info(`gfn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism I don't think this uvm_info
call should be indented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, here the formatter is showing you the difference between writing:
wait(x);
statement;
and
wait(x)
statement;
These are syntactically different, though they may give you the same result semantically. (A language lawyer could speak to this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! this seems fine then.
alert_host_agent[i].monitor.alert_esc_port.connect( | ||
scoreboard.alert_fifo[i].analysis_export); | ||
alert_host_agent[i | ||
].monitor.alert_esc_port.connect(scoreboard.alert_fifo[i].analysis_export); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfschaffner @fangism this should be formatted as
alert_host_agent[i].monitor.alert_esc_port.connect(
scoreboard.alert_fifo[i].analysis_export);
(I'm assuming this line exceeds the acceptable length)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, but the next iteration should leave this alone.
@@ -92,16 +90,18 @@ class alert_handler_scoreboard extends cip_base_scoreboard #( | |||
if (act_item.alert_esc_type == AlertEscSigTrans && !act_item.timeout && | |||
act_item.alert_handshake_sta == AlertReceived) begin | |||
process_alert_sig(index, 0); | |||
// alert integrity fail | |||
// alert integrity fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism this is another interesting comment scenario - this comment applies to the line below, which is why it wasn't inline with the process_alert_sig(index, 0)
line - so we want to keep the existing indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the intent. Without reading the comment like a human, I'm not sure it is possible for a tool to infer this.
(act_item.esc_handshake_sta == EscIntFail && !act_item.timeout)) begin | ||
// escalation integrity fail | ||
end else if (act_item.alert_esc_type == AlertEscIntFail || ( | ||
act_item.esc_handshake_sta == EscIntFail && !act_item.timeout)) begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism another case where there is a strange linebreak right after an opening parenthesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be left alone in the next iteration.
// if class escalation is enabled, add alert to accumulation count | ||
if (class_ctrl[AlertClassCtrlEn] && | ||
(class_ctrl[AlertClassCtrlEnE3:AlertClassCtrlEnE0] > 0)) begin | ||
if (class_ctrl[AlertClassCtrlEn] && (class_ctrl[AlertClassCtrlEnE3:AlertClassCtrlEnE0] > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism I think we should keep the existing formatting here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This will be left untouched in next round.
for (int sig_i = 0; sig_i < NUM_ESC_SIGNALS; sig_i++) begin | ||
if (class_ctrl[sig_i*2+7 -: 2] == phase_i && class_ctrl[sig_i+2]) begin | ||
enabled_sig_q.push_back(sig_i); | ||
forever |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism I think we want the original formatting here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@udinator Interesting, so forever
is considered merge-able with the construct it encloses. Can you file a new issue for this one?
@@ -108,8 +108,8 @@ class alert_handler_base_vseq extends cip_base_vseq #( | |||
if (esc_int_errs[index]) begin | |||
fork | |||
begin | |||
esc_receiver_esc_rsp_seq esc_seq = | |||
esc_receiver_esc_rsp_seq::type_id::create("esc_seq"); | |||
esc_receiver_esc_rsp_seq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism I think we should keep original formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this will be left alone because it wraps (next iteration).
constraint esc_accum_thresh_c { | ||
foreach (accum_thresh[i]) {accum_thresh[i] inside {[0:100]};} | ||
} | ||
constraint esc_accum_thresh_c {foreach (accum_thresh[i]) {accum_thresh[i] inside {[0 : 100]};}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism I think we should keep the original formatting here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wr_reg({26'b0, ENTROPY_SRC_ES_SEED_OFFSET}, 32'h1111_1111); // seed | ||
wr_reg({26'b0, ENTROPY_SRC_ES_CONF_OFFSET}, 32'h0000_0001); // primary enable | ||
repeat (10) @(posedge clk); | ||
wr_reg({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism @msfschaffner I'm not sure that we want to split the stuff inside the brackets onto new lines here?
FlashMemInitSet, // Initialize with all 1s. | ||
FlashMemInitClear, // Initialize with all 0s. | ||
FlashMemInitRandomize, // Initialize with random data. | ||
FlashMemInitCustom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism @msfschaffner this appears to be some strange formatting going on , I think we should keep the original formatting here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, the original formatting would be preferred here.
output logic[EscCntDw-1:0] esc_cnt_o, | ||
output logic[N_ESC_SEV-1:0] esc_sig_en_o, | ||
output cstate_e esc_state_o | ||
module alert_handler_esc_timer_fpv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cindychip how would you normally want this formatted?
@fangism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Udi for asking, I think "module_name"_fpv sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Also, what do you think about the way the import alert_pkg::*
gets formatted to a newline? Is this something you want to keep, or would you rather keep the original way you formatted it all inline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, I did not notice that. I do not have a preference. I think either way works. Personally I would think having a new line for import is easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the input!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@udinator Can you file a new issue for this? And do you want the import
s indented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I'll file a bug.
Ideally we want the import
s indented also.
FYI, I created a formatting with the current |
initial // clock generation | ||
begin | ||
clk = 0; | ||
forever begin | ||
#4ns clk = !clk; | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism seems like the begin
keyword got an extra indent while the contents inside got left-indented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@udinator Can you file a new issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same as earlier)
rst_n = 1; | ||
end | ||
initial // reset generation | ||
begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism, I think this is similar to above, the begin
keyword is getting indented again for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same indeed.
illegal_bins data_oe_1_data_out_0_data_in_1 = binsof (data_out) intersect { | ||
0 | ||
} && binsof (data_oe) intersect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fangism another case of brackets getting spread out over three lines, but this time inside of covergroups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@udinator Can you file a new issue?
Experimental PR to see and comment on diffs.