[libcxx-commits] [PATCH] D153140: [libc++][NFC] Apply clang-format on large parts of the code base

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__type_traits/is_convertible.h:52
 
+// clang-format off
 template <class _Tp,
----------------
philnik wrote:
> ldionne wrote:
> > philnik wrote:
> > > In this case I don't get why you didn't just clang-format it. Also for the similar ones below.
> > 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.
> ```
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153140/new/

https://reviews.llvm.org/D153140