llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.95k stars 11.53k forks source link

Aligning consecutive assignments in enum blocks only #52983

Open Julien-Elie opened 2 years ago

Julien-Elie commented 2 years ago

Could it be possible to have an option to align assignments in enum blocks?

For instance:

enum nntp_code {
    NNTP_INFO_HELP         = 100,
    NNTP_INFO_CAPABILITIES = 101,
    NNTP_INFO_DATE         = 111,
    NNTP_OK_BANNER_POST    = 200,
    NNTP_OK_BANNER_NOPOST  = 201,
    NNTP_OK_QUIT           = 205,
    NNTP_OK_COMPRESS       = 206,
    NNTP_OK_GROUP          = 211,
    NNTP_OK_LIST           = 215,
    NNTP_OK_ARTICLE        = 220,
    NNTP_OK_HEAD           = 221,
    NNTP_OK_BODY           = 222,
}
mydeveloperday commented 2 years ago

Can you explain why this isn't just AlignConsecutiveAssignments: Consecutive

which gives

enum nntp_code {
  NNTP_INFO_HELP         = 100,
  NNTP_INFO_CAPABILITIES = 101,
  NNTP_INFO_DATE         = 111,
  NNTP_OK_BANNER_POST    = 200,
  NNTP_OK_BANNER_NOPOST  = 201,
  NNTP_OK_QUIT           = 205,
  NNTP_OK_COMPRESS       = 206,
  NNTP_OK_GROUP          = 211,
  NNTP_OK_LIST           = 215,
  NNTP_OK_ARTICLE        = 220,
  NNTP_OK_HEAD           = 221,
  NNTP_OK_BODY           = 222,
}
Julien-Elie commented 2 years ago

I agree AlignConsecutiveAssignments does the job in enum but I would just like to align assignments in enum, not everywhere in the source code.

mydeveloperday commented 2 years ago

This is doable but would require careful backwards compatibility considerations

llvmbot commented 1 year ago

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1) Assign the issue to you. 2) Fix the issue locally. 3) Run the test suite locally. 3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests. 4) Create a git commit 5) Run git clang-format HEAD~1 to format your changes. 6) Submit the patch to Phabricator. 6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

gapisback commented 7 months ago

Hello, Gentlepeople, I just started looking at LLVM git repo and this set of open issues.

This one to do with clang-format-ting is interesting to me as it's a non-core, non-compilation code change. I am trying to teach myself compiler development, so would be interesting to work on this kind of outside the actual clang change.

I'm aware of the backwards-compatibility issue(s) and will take a crack at specifying the interface and behaviour once this issue is assigned to me.

Seems like there is some re-thinking on whether this is a 'new issue' or 'good first issue'; attn. @Endilll -- is this really that difficult?

[ @Julien-Elie - do you have ownership to assign? ] Will wait for the assigment to happen before engaging the people who commented on this issue with next-steps and a proposal.

Julien-Elie commented 7 months ago

Thanks for having a look at this ticket (as well as other tickets). Greatly appreciated to go on improving clang-format.

gapisback commented 7 months ago

Thank you @Endilll for the quick assignment. I'll start looking at it, @Julien-Elie

This work-item affects clang-formatting rules and the output of formatted code. There is a bit of backward compatibility too. As this is my very 1st foray into changes in this repo, just asking what the process is here. A specific question:

In past such experiences, elsewhere, when interfaces & output/functionality is involved, I prefer to air out an initial 'mini-spec' describing the new / changed interface and how the affected output will be. Some code-owners sign-off on the interface 'mini-spec', before I turn out the code changes and submit a PR.

Even though this issue is a small'ish item, I'd like to follow that process as it avoids code rework due to interface changes.

Is this team equipped for such a round-trip pre-review of interfaces? If so, who would I get sign-off from? (I'll find the last author(s) who worked clang-format tool by git blame, but thought I'd ask here if anyone has a quick suggestion of names to run the proposal by.)

Julien-Elie commented 3 months ago

@gapisback, did you finally manage to have a look at it?

Julien-Elie commented 2 months ago

@mydeveloperday About your suggestion of using AlignConsecutiveAssignments (though it applies to all assignments, and not only enum), I'm wondering whether it could not be useful to allow specific formatting in some parts of the code.

I explain: instead of totally disabling the reformatting with /* clang-format off */, couldn't instead something like /* clang-format +"AlignConsecutiveAssignments: AcrossEmptyLinesAndComments" */ be used, and when I no longer want it /* clang-format -"AlignConsecutiveAssignments: AcrossEmptyLinesAndComments" */? I don't know whether such a change of style is technically doable within a give file (it may be hard to implement) and if it had already been discussed before.

jm4R commented 2 weeks ago

This idea is a rocket!