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

Formatter: Align multiline string initializer #834

Merged

Conversation

mglb
Copy link
Contributor

@mglb mglb commented May 25, 2021

Demo:

string seq_names[] = {"uart_sanity_vseq",
                      "uart_tx_rx_vseq",
                      "uart_fifo_full_vseq",
                      "uart_fifo_overflow_vseq",
                      "uart_fifo_reset_vseq",
                      "uart_common_vseq",  // for intr_test
                      "uart_intr_vseq",
                      "uart_noise_filter_vseq",
                      "uart_rx_start_bit_filter_vseq",
                      "uart_rx_parity_err_vseq",
                      "uart_tx_ovrd_vseq",
                      "uart_perf_vseq",
                      "uart_loopback_vseq"};
string abc[] = {"a",
                "b",  //
                "c"};
string abc[] = {"a",
                "b",  //
                "c"  //
                };
string abc[] = {  //
  "a",
  "b",
  "c"
};
string seq_names_without_comments[] = {"uart_sanity_vseq",
                                       "uart_tx_rx_vseq",
                                       "uart_fifo_full_vseq",
                                       "uart_fifo_overflow_vseq",
                                       "uart_fifo_reset_vseq",
                                       "uart_common_vseq",
                                       "uart_intr_vseq",
                                       "uart_noise_filter_vseq",
                                       "uart_rx_start_bit_filter_vseq",
                                       "uart_rx_parity_err_vseq",
                                       "uart_tx_ovrd_vseq",
                                       "uart_perf_vseq",
                                       "uart_loopback_vseq"};
string abc_without_comments[] = {"a", "b", "c"};
string seq_names_width_doubled_strings_and_a_variable_name_that_is_too_long[] = {
  "uart_sanity_vseq uart_sanity_vseq",
  "uart_tx_rx_vseq uart_tx_rx_vseq",
  "uart_fifo_full_vseq uart_fifo_full_vseq",
  "uart_fifo_overflow_vseq uart_fifo_overflow_vseq",
  "uart_fifo_reset_vseq uart_fifo_reset_vseq",
  "uart_common_vseq uart_common_vseq",  // for intr_test
  "uart_intr_vseq uart_intr_vseq",
  "uart_noise_filter_vseq uart_noise_filter_vseq",
  "uart_rx_start_bit_filter_vseq uart_rx_start_bit_filter_vseq",
  "uart_rx_parity_err_vseq uart_rx_parity_err_vseq",
  "uart_tx_ovrd_vseq uart_tx_ovrd_vseq",
  "uart_perf_vseq uart_perf_vseq",
  "uart_loopback_vseq uart_loopback_vseq"
};
string aaaabbbbcccc[] = {"aaaaaaaaaaaaaaaaaaaaaaaa",
                         "bbbbbbbbbbbbbbbbbbbbbbbb",  //
                         "cccccccccccccccccccccccc"};
string aaaabbbbcccc[] = {"aaaaaaaaaaaaaaaaaaaaaaaa",
                         "bbbbbbbbbbbbbbbbbbbbbbbb",
                         "cccccccccccccccccccccccc"};
string seq_names_with_non_string_item[] = {
  "uart_sanity_vseq",
  UART_TX_RX_VSEQ,
  "uart_fifo_full_vseq",
  "uart_fifo_overflow_vseq",
  "uart_fifo_reset_vseq",
  "uart_common_vseq",  // for intr_test
  "uart_intr_vseq",
  "uart_noise_filter_vseq",
  "uart_rx_start_bit_filter_vseq",
  "uart_rx_parity_err_vseq",
  "uart_tx_ovrd_vseq",
  "uart_perf_vseq",
  "uart_loopback_vseq"
};
string abc[] = {
  "a",
  B,  // non-string-literal
  "c"
};
string abc[] = {"a", B, "c"};
string nesting[] = {
  "uart_sanity_vseq",
  "uart_tx_rx_vseq",
  {
    "uart_fifo_full_vseq",
    "uart_fifo_overflow_vseq",
    "uart_fifo_reset_vseq",
    "uart_common_vseq",  // for intr_test
    "uart_intr_vseq",
    "uart_noise_filter_vseq",
    "uart_rx_start_bit_filter_vseq",
    "uart_rx_parity_err_vseq",
    "uart_tx_ovrd_vseq"
  },
  "uart_perf_vseq",
  "uart_loopback_vseq"
};
string abcd[] = {
  "a",
  {
    "b",  //
    "c"
  },
  "d"
};
string abcd[] = {"a", {"b", "c"}, "d"};
string seq_names_with_long_non_first_item[] = {
  "uart_sanity_vseq",
  "uart_tx_rx_vseq",
  "uart_fifo_full_vseq",
  "uart_fifo_overflow_vseq",
  "uart_fifo_reset_vseq",
  "uart_common_vseq",
  "uart_intr_vseq",
  "uart_noise_filter_vseq",
  "uart_rx_start_bit_filter_vseq aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
  "uart_rx_parity_err_vseq",
  "uart_tx_ovrd_vseq",
  "uart_perf_vseq",
  "uart_loopback_vseq"
};

Closes #41

@googlebot googlebot added the cla: yes All contributors in pull request have signed the CLA with Google. label May 25, 2021
@mglb mglb added the formatter Verilog code formatter issues label May 25, 2021
@mglb mglb force-pushed the mglb/MultilineStringInitializer branch from 3138505 to dab41ee Compare May 25, 2021 22:41
@mglb mglb force-pushed the mglb/MultilineStringInitializer branch from dab41ee to f69ae47 Compare June 4, 2021 13:55
@mglb mglb force-pushed the mglb/MultilineStringInitializer branch 5 times, most recently from 2ba989a to 60dc7a8 Compare June 10, 2021 00:43
Mariusz Glebocki added 4 commits June 10, 2021 14:38
The partitions in "wrapped first subpartition" mode use their existing
indentation now. This has been done to support different use cases, e.g.
wrapped function arguments (8 spaces), or strings inside concatenation
expressions (4 spaces).
@mglb mglb force-pushed the mglb/MultilineStringInitializer branch from 60dc7a8 to 8c2f1db Compare June 10, 2021 12:45
@mglb mglb marked this pull request as ready for review June 10, 2021 13:30
@mglb mglb requested a review from hzeller June 10, 2021 13:30
@mglb mglb changed the title [WIP] Formatter: Align multiline string initializer Formatter: Align multiline string initializer Jun 10, 2021
@tgorochowik
Copy link
Member

@hzeller could you please take a look at this too?

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

Nice!

// Wrapped first argument
wrapped_first_subpartition = true;
} else {
bool wrapped_first_subpartition;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Below, this value is essentially depending on the result of the !wrap_first_subpartition && (fit_result = FitsOnLine(first_line, style)).fits == true) if condition (if I see it correctly).
So maybe we can assign the value direclty and make it const.

  const bool wrapped_first_subparition = ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{"string abc[] = {//\n"
"\"a\", \"b\", \"c\""
"};\n",
"string abc[] = { //\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if some of these strings would be a bit more readable using c++11 raw string literals to have the expected string shine through more with less escape-backslashes needed.
In general, the strings are fine, leaving it up to you to experiment with that option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raw literals could be used only for strings that don't have \n - otherwise we'll need an extra "\n" which are put in separate lines by autoformatter. I think it's better to avoid those few "different" lines and keep everything uniform.
Or did you mean multiline raw strings? Like:

    {R"(string abc[]= {//
"a", "b", "c"
};
)",
     R"(string  abc[] = {  //
  "a",
  "b",
  "c"
};
)"}

@tgorochowik
Copy link
Member

I think we can merge this

@tgorochowik tgorochowik merged commit 99ce749 into chipsalliance:master Jul 27, 2021
@tgorochowik tgorochowik deleted the mglb/MultilineStringInitializer branch July 27, 2021 15:57
@hzeller
Copy link
Collaborator

hzeller commented Aug 25, 2021

This crashes when run over all ibex files; in the following example https://github.com/lowrisc/ibex was checked out to /tmp/ibex/

bazel build verilog/tools/formatter:verible-verilog-format
bazel-bin/verilog/tools/formatter/verible-verilog-format --inplace $(find /tmp/ibex -name "*.sv")
F20210825 14:07:17.748425 152869 tree_utils.h:75] Check failed: E(node.Tag().tag) == expected_node_enum (kClassNew vs. kExpression) 
*** Check failure stack trace: ***
    @     0x55e080ced280  google::LogMessage::Fail()
    @     0x55e080ced1cb  google::LogMessage::SendToLog()
    @     0x55e080cecaf2  google::LogMessage::Flush()
    @     0x55e080cf02d8  google::LogMessageFatal::~LogMessageFatal()
    @     0x55e080bf1658  verible::CheckNodeEnum<>()
[...]

Bisecting shows that this is the commit where that happens. Can you roll back @tgorochowik @mglb ? (since there were other commits afterwards, it is not a simple reverse patch unfortunately).

@hzeller
Copy link
Collaborator

hzeller commented Aug 25, 2021

(or fixing and roll-forward is also an option of course)

@tgorochowik
Copy link
Member

Sure, we'll take a look at this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes All contributors in pull request have signed the CLA with Google. formatter Verilog code formatter issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[line breaks, wraps] Multiline string initializer
4 participants