[libcxx-commits] [PATCH] D153140: [libc++][NFC] Apply clang-format on large parts of the code base
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jun 19 08:19:34 PDT 2023
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
More information about the libcxx-commits
mailing list