This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] clang-format <__config>
ClosedPublic

Authored by philnik on Jun 13 2022, 7:51 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGac251726f84d: [libc++][NFC] clang-format <__config>
Summary

It's not perfect, but it's a lot better than the status quo.

Diff Detail

Event Timeline

philnik created this revision.Jun 13 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 7:51 AM
philnik requested review of this revision.Jun 13 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 7:52 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jun 13 2022, 8:03 AM
ldionne added inline comments.
libcxx/include/__config
713–735

I wouldn't make this part of the change, I think it makes things strictly worse. Perhaps // clang-format off?

798–799

The semi-colon alone on its own line is crazy, let's put it on the same line as the }.

973–977

These changes make it harder to read the conditions, and quite importantly, they greatly increase the risk of conflicts between upstream and downstream. Having one platform per line is actually quite useful because it's more like a list and it reduces the likelihood of having merge conflicts downstream. We should keep it as-is.

1046

Same here, keep on individual lines.

This revision now requires changes to proceed.Jun 13 2022, 8:03 AM
philnik updated this revision to Diff 436407.Jun 13 2022, 8:25 AM
philnik marked 4 inline comments as done.
  • Address comments
EricWF added a subscriber: EricWF.Jun 13 2022, 11:18 AM

I really don't know if this is worth the cost of losing all the blame history.
I know you can get around whitespace changes, but still.

I think we should just incrementally format this as it gets changed.

ldionne accepted this revision.Jun 13 2022, 2:52 PM

I really don't know if this is worth the cost of losing all the blame history.
I know you can get around whitespace changes, but still.

I think we should just incrementally format this as it gets changed.

I actually just tried pulling this patch and adding the commit to .git-blame-ignore-revs, and it's basically as-if this commit had never been done, so it's not too disruptive. I think __config is somewhat special in that these changes are almost exclusively whitespace-only changes, which git blame can deal with really well. This wouldn't be the case for refactoring most other headers.

Given that the alternative is to get a bunch of formatting changes mixed into day-to-day reviews, I think I'd rather do this file all at once and add the commit to git-blame-ignore-revs.

This revision is now accepted and ready to land.Jun 13 2022, 2:52 PM
This revision was landed with ongoing or failed builds.Jun 13 2022, 9:47 PM
This revision was automatically updated to reflect the committed changes.