This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Reformat builtins with clang-format
ClosedPublic

Authored by phosek on Apr 5 2019, 6:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.Apr 5 2019, 6:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 5 2019, 6:10 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 7 others. · View Herald Transcript
phosek retitled this revision from [compiler-rt] Reformat builtins with clang-format to [builtins] Reformat builtins with clang-format.Apr 5 2019, 6:10 PM
thakis added a subscriber: thakis.Apr 5 2019, 7:45 PM
thakis added inline comments.
compiler-rt/lib/builtins/lshrti3.c
31 ↗(On Diff #193991)

clang-format interacts poorly with some of the comments in these files. Maybe look through the diff yourself locally, there's a few places that could use manual cleaning up.

jfb added a comment.Apr 6 2019, 2:38 PM

I'd much rather see format updates happen as the files are touched, instead of all-out like this, because it makes mergebots sad.

jfb added a reviewer: jfb.Apr 6 2019, 2:50 PM
In D60351#1457455, @jfb wrote:

I'd much rather see format updates happen as the files are touched, instead of all-out like this, because it makes mergebots sad.

Huh, usually the guidance is "land reformat in a separate commit". And we've mass-reformated code that didn't follow LLVM style before, e.g. lldb. What's different here?

jfb added a comment.Apr 7 2019, 1:54 PM
In D60351#1457455, @jfb wrote:

I'd much rather see format updates happen as the files are touched, instead of all-out like this, because it makes mergebots sad.

Huh, usually the guidance is "land reformat in a separate commit". And we've mass-reformated code that didn't follow LLVM style before, e.g. lldb. What's different here?

Do you intend to change all these files in the near future? If we want to go by guidance:

"Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to. Also, try to separate formatting or whitespace changes from functional changes, either by correcting the format first (ideally) or afterward. Such changes should be highly localized and the commit message should clearly state that the commit is not intended to change functionality, usually by stating it is NFC." from https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Agreed that format changes should happen as NFC commits, but I'm just not a fan of commits that just do that because the code is "wrong". Unless there's subsequent changes coming to each file, it's useless churn that hurts mergebots and makes the history annoying to follow.

Not that this is "over my dead body". I'm just asking if the motivation is just "oh hey it's wrongly formatted, let's fix the format". If so, I'm not a fan. I'd much rather fix code as we change it.

jfb added a comment.Apr 8 2019, 9:21 AM

Thanks @echristo for pointing out "[RFC] compiler-rt builtins cleanup and refactoring". You do intend to modify this code, so a reformat ahead of time is then fine. Please link to the RFC in the commit message.

echristo accepted this revision.Apr 8 2019, 10:03 AM

LGTM.

This revision is now accepted and ready to land.Apr 8 2019, 10:03 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Jul 12 2019, 9:53 AM
nikic added inline comments.
compiler-rt/trunk/lib/builtins/arm/sync-ops.h
37

It looks like this reformatted to illegal assembly:

compiler-rt/lib/builtins/arm/sync_fetch_and_add_8.S:21: Error: bad instruction `push{r4, r5,r6,lr}'

The whitespace between push and { likely needs to be preserved.

nikic added inline comments.Jul 12 2019, 2:12 PM
compiler-rt/trunk/lib/builtins/arm/sync-ops.h
37

I've added back the space in rL365957.