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

[ARM][AArch64] Reformat target parser. NFC #82601

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

davemgreen
Copy link
Collaborator

This is something we generally tend to avoid due to confusing the git history, but with the new github formatting bots being more noisy we keep running into issues with the existing formatting when adding or adjusting CPUs. This patch formats the code to make sure we are in a good state going forward. I can tone this down if needed, reducing some of the changes that are outside of the main cpu tables (especially the .def). I just ran clang-format on the altered files.

@DavidSpickett
Copy link
Collaborator

This is something we generally tend to avoid due to confusing the git history

Add this to https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs, that'll mitigate that.

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

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

LGTM, I've thought about this before but not had the stomach for the merge conflict it'll create. A few places are less pretty but still readable, and some more readable, so I'm happy to go with clang-format's decisions here.

@jthackray should do the final approval just to confirm that landing this now isn't going to be bad timing for their team downstream.

@davemgreen
Copy link
Collaborator Author

Good point - I can try and help sorting out the merge conflict too.

@jthackray
Copy link
Contributor

We've branched our latest release a couple of weeks ago, but are still doing occasional cherry-picks (and I've got pending upstream updates). Would prefer if we could leave until Monday; I'm still catching up as I've been away earlier this week due to half-term holidays.

But yes, it's a good idea to tidy this area of the code up :)

@davemgreen
Copy link
Collaborator Author

Yep - certainly happy to wait. I will give this a rebase this some time next week

Copy link
Contributor

@jthackray jthackray left a comment

Choose a reason for hiding this comment

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

LGTM.

This is something we generally tend to avoid due to confusing the git history,
but with the new github formatting bots being more noisy we keep running into
issues with the existing formatting when adding or adjusting CPUs. This patch
formats the code to make sure we are in a good state going forward. I can tone
this down if needed, reducing some of the changes that are outside of the main
cpu tables. I just ran clang-format on the altered files.
@davemgreen davemgreen merged commit 800de14 into llvm:main Mar 3, 2024
4 checks passed
@davemgreen davemgreen deleted the gh-formattargetparser branch March 3, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants