It's not perfect, but it's a lot better than the status quo.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGac251726f84d: [libc++][NFC] clang-format <__config>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__config | ||
---|---|---|
718–735 | I wouldn't make this part of the change, I think it makes things strictly worse. Perhaps // clang-format off? | |
797–798 | The semi-colon alone on its own line is crazy, let's put it on the same line as the }. | |
974–1013 | 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. | |
1064–1066 | Same here, keep on individual lines. |
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.
I wouldn't make this part of the change, I think it makes things strictly worse. Perhaps // clang-format off?