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

clang-format change: one arg per line for long function call/def #1126

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented Nov 13, 2024

Description

Change clang-format config so that long function call/def will have each argument taking a new line, with brackets in their own lines.

The current formatter breaks only when a line is filled, causing multiple parameters sharing the same line, which creates walls of code that are not easy to read. This should mitigate the issue.

Drawback: A few super long template from ac_math.h looks bad now, but as they are not that much better before I guess it's a reasonable tradeoff...

Example:

Current:

template <class data_T, class res_T, typename CONFIG_T>
void dense(data_T data[CONFIG_T::n_in], res_T res[CONFIG_T::n_out],
           typename CONFIG_T::weight_t weights[CONFIG_T::n_in * CONFIG_T::n_out],
           typename CONFIG_T::bias_t biases[CONFIG_T::n_out]) {
    #pragma HLS INLINE
    CONFIG_T::template kernel<data_T, res_T, CONFIG_T>::dense(data, res, weights, biases);
}

Modified:

template <class data_T, class res_T, typename CONFIG_T>
void dense(
    data_T data[CONFIG_T::n_in],
    res_T res[CONFIG_T::n_out],
    typename CONFIG_T::weight_t weights[CONFIG_T::n_in * CONFIG_T::n_out],
    typename CONFIG_T::bias_t biases[CONFIG_T::n_out]
) {
    #pragma HLS INLINE
    CONFIG_T::template kernel<data_T, res_T, CONFIG_T>::dense(data, res, weights, biases);
}

Type of change

For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.

Note: Please delete options that are not relevant.

  • Cosmatic

Tests

📝 Please describe the tests that you ran to verify your changes.

  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Test Configuration:

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@calad0i calad0i changed the title Multiline cpp fn def clang-format change: one arg per line for long function call/def Nov 13, 2024
@vloncar
Copy link
Contributor

vloncar commented Nov 15, 2024

I agree that the function declarations look nicer this way, that used to be the preferred-but-not-enforced style before we started using clang-format. However, the template declaration now looks really ugly.

Also, after hours of fiddling, I never managed to get labels to be indented and align to the code (in our case the loop), instead they are always aligned at the level of the surrounding brace. This is the biggest eyesore in the current codebase. Any suggestions as to how to fix that? Seems to be doable with astyle, but clang-format is more powerful and generally recommended, so I'd rather not switch the formatter

@jmduarte jmduarte added the please test Trigger testing by creating local PR branch label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants