This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Apply clang-format on large parts of the code base
ClosedPublic

Authored by ldionne on Jun 16 2023, 6:50 AM.

Details

Summary

This commit does a pass of clang-format over files in libc++ that
don't require major changes to conform to our style guide, or for
which we're not overly concerned about conflicting with in-flight
patches or hindering the git blame.

This roughly covers:

  • benchmarks
  • range algorithms
  • concepts
  • type traits

I did a manual verification of all the changes, and in particular I
applied clang-format on/off annotations in a few places where the
result was less readable after than before. This was not necessary
in a lot of places, however I did find that clang-format had pretty
bad taste when it comes to formatting concepts.

Diff Detail

Event Timeline

ldionne created this revision.Jun 16 2023, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 6:50 AM
ldionne requested review of this revision.Jun 16 2023, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 6:50 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision.Jun 16 2023, 7:35 AM
philnik added a subscriber: philnik.
philnik added inline comments.
libcxx/benchmarks/Utilities.h
29
libcxx/benchmarks/allocation.bench.cpp
20–21

Maybe as a quick follow-up?

This revision is now accepted and ready to land.Jun 16 2023, 7:35 AM
Mordante accepted this revision.Jun 16 2023, 8:56 AM
Mordante added a subscriber: Mordante.

LGTM! After committing can you do a follow-up commit, adding this commit to .git-blame-ignore-revs?

I noticed in the past we have quite some headers in include that are "almost-formatted" (diff < 25-ish lines) would it make sense to do these too?

In general I wonder whether we should be slightly more aggressive in removing this tech debt. I know we want to avoid disrupting active patches, which I think is great. OTOH we have quite some inactive places were we could do
_VSTD -> std
typedef -> using
_LIBCPP_INLINE_VISIBILITY -> _LIBCPP_HIDE_FROM_ABI

This is something we need to point out in reviews quite often or get questions about it.

We still have a lot of old patches, but I think we need quite some effort to rebase them anyway; and I'm not even sure whether to original authors are intending to pursue these patches.

ldionne marked 2 inline comments as done.Jun 16 2023, 10:18 AM

@Mordante I will expand the scope of this patch to touch more headers. I won't do everything because I want to avoid touching active areas and manually auditing everything is time consuming, but I've got a patch that applies clang-format to a couple hundred files.

libcxx/benchmarks/allocation.bench.cpp
20–21

Will do in a separate patch.

ldionne updated this revision to Diff 532210.Jun 16 2023, 10:19 AM
ldionne marked an inline comment as done.
ldionne retitled this revision from [libc++][NFC] Reformat all the benchmarks to [libc++][NFC] Apply clang-format on large parts of the code base.
ldionne edited the summary of this revision. (Show Details)

Expand scope.

ldionne added inline comments.Jun 16 2023, 10:21 AM
libcxx/include/__concepts/copyable.h
27–28

For some concepts where I judged that alignment was really helping readability, I added on/off annotations. In other places I didn't because I felt like readability was less affected, but this is arguably a judgement call.

Mordante accepted this revision.Jun 16 2023, 10:41 AM

@Mordante I will expand the scope of this patch to touch more headers. I won't do everything because I want to avoid touching active areas and manually auditing everything is time consuming, but I've got a patch that applies clang-format to a couple hundred files.

Thanks! I looked over the files and didn't spot bad formatting.

libcxx/include/__type_traits/invoke.h
355–357

Good old C++03 ;-)

libcxx/include/__type_traits/is_scalar.h
21

Odd it changed it here, but I don't see issues with the change.

ldionne added inline comments.Jun 16 2023, 11:30 AM
libcxx/include/__type_traits/invoke.h
355–357

Yeah, that's annoying! Is this a request to change this instance and other ones? I'm not sure it's possible because clang-format will try to enforce that (unfortunate) whitespace.

libcxx/include/__type_traits/is_scalar.h
21

I played around a bit and it looks like clang-format thinks that this file is objective-C because of the block syntax, and our .clang-format file doesn't apply to Objc. I'd like to address that in a separate patch.

ldionne updated this revision to Diff 532253.Jun 16 2023, 11:43 AM

Rebase onto CI fixes

Mordante added inline comments.Jun 16 2023, 12:32 PM
libcxx/include/__type_traits/invoke.h
355–357

Fixing it will refrain clang-format from touching it. Feel free to ignore it.

libcxx/include/__type_traits/is_scalar.h
21

A cool you found a cause. I was quite baffled by it. I initially was concerned there was an issue with the code.

philnik added inline comments.Jun 18 2023, 3:38 PM
libcxx/include/__concepts/copyable.h
27–28

I'm fine with having a few // clang-format on/off in the code. clang-format is for the 99% where there isn't a readability problem and we just need some formatting decision.

libcxx/include/__type_traits/decay.h
49
libcxx/include/__type_traits/invoke.h
355–357

Yeah, this is the SpacesInAngles: Leave. It won't remove any whitespace tokens, but also won't add any.

libcxx/include/__type_traits/is_convertible.h
52

In this case I don't get why you didn't just clang-format it. Also for the similar ones below.

ldionne updated this revision to Diff 532631.Jun 19 2023, 6:26 AM
ldionne marked 8 inline comments as done.

Apply comments.

libcxx/include/__type_traits/invoke.h
355–357

Ah, I understand now. Ok, I fixed it.

libcxx/include/__type_traits/is_convertible.h
52

It's really helpful to align these definitions:

template <class _Tp> struct __is_array_function_or_void<_Tp, true, false, false> { enum { value = 1 }; };
template <class _Tp> struct __is_array_function_or_void<_Tp, false, true, false> { enum { value = 2 }; };
template <class _Tp> struct __is_array_function_or_void<_Tp, false, false, true> { enum { value = 3 }; };

Otherwise, which specialization handles which combination of is_array/is_function/is_void isn't nearly as clear. A similar reasoning applies in other places.

I've left things as-is.

philnik added inline comments.Jun 19 2023, 8:09 AM
libcxx/include/__type_traits/is_convertible.h
52
template <class _Tp>
struct __is_array_function_or_void<_Tp, true, false, false> {
  enum { value = 1 };
};
template <class _Tp>
struct __is_array_function_or_void<_Tp, false, true, false> {
  enum { value = 2 };
};
template <class _Tp>
struct __is_array_function_or_void<_Tp, false, false, true> {
  enum { value = 3 };
};

is the formatted version, which looks just as readable to me. the true, false, false etc. are still aligned.

ldionne added inline comments.Jun 19 2023, 8:19 AM
libcxx/include/__type_traits/is_convertible.h
52

I agree that when looked at in isolation, both are comparable in readability. But when they appear in the file as a whole, it becomes a lot less clear that these declarations are related when each of them uses 4 lines than when they are each a one-liner. I'll land the patch as-is and we can discuss this more and change it in a follow-up patch, but I'd like to avoid merge conflicts by delaying this based on something that's easy to change.