Page MenuHomePhabricator

[NFC][libc++] Update clang-format style.
ClosedPublic

Authored by Mordante on Sep 15 2021, 9:31 AM.

Details

Reviewers
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Commits
rG15dfe7834062: [NFC][libc++] Update clang-format style.
Summary

Changes the style as requested by @ldionne in D103368.

Diff Detail

Event Timeline

Mordante requested review of this revision.Sep 15 2021, 9:31 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 9:31 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Quuxplusone.

LGTM. (Although you know I think clang-format is useless for libc++ anyway.)

Thanks. I still hope we can use clang-format after the LLVM-13 release. This should address our > > issue.

ldionne requested changes to this revision.Sep 20 2021, 5:25 PM
ldionne added inline comments.
libcxx/.clang-format
16

Does this mean it'll always be only indented by one space? What we need is to indent it to tabs, whatever the size of tabs are in a given file. Is there a way to achieve that?

This revision now requires changes to proceed.Sep 20 2021, 5:25 PM

Sorry I forgot the submit my comment before.

libcxx/.clang-format
16

I assume you mean our standard indention of 2 spaces with "tabs". That would be achieved by removing this line, then it uses the default indention. I added this since you requested a one space indention D103368 and I've seen that indention used before in libc++.

ldionne added inline comments.Sep 23 2021, 8:56 AM
libcxx/.clang-format
16

Sorry if we misunderstood each other -- what I want is for #ifs to be indented consistently with the surrounding indentation, period. So if the surrounding indentation uses 4 tabs, nested preprocessor directives should look like:

#if CONDITION
#   define FOO
    ^ 5th column, i.e. indentation consists of 1x'#' and 3x' '
#endif

Similarly, if surrounding indentation uses 2 spaces, then it should look like:

#if CONDITION
# define FOO
  ^ 3rd column, i.e. indentation consists of 1x'#' and 1x' '
#endif

Does that make sense?

ldionne added inline comments.Sep 23 2021, 8:57 AM
libcxx/.clang-format
16

So if the surrounding indentation uses 4 tabs 4 spaces

(just to clarify what I meant)

libcxx/.clang-format
16

FWIW, bleagh. I've never seen libc++ do

void f() {
#if SOMETHING
    int x;
#    if SOMETHINGELSE
    int y;
#    endif // SOMETHINGELSE
#endif // SOMETHING
}

nor do I think that's a good style. I think that in running code libc++'s style is generally "no indents at all":

void f() {
#if SOMETHING
    int x;
#if SOMETHINGELSE
    int y;
#endif // SOMETHINGELSE
#endif // SOMETHING
}

and in preprocessor code it's generally "one space":

#if SOMETHING
# define X y
# if SOMETHINGELSE
#  define Y z
# endif // SOMETHINGELSE
#endif // SOMETHING

Of course as usual clang-format can't handle that complicated of a rule (it has no notion of "preprocessor code" versus "normal code"). Ultimately we just have to just pick something for the .clang-format file and advise people to ignore it if it seems to be doing the wrong thing.

ldionne added inline comments.Sep 23 2021, 10:58 AM
libcxx/.clang-format
16

My comment only referred to "preprocessor code", not "normal code". Indeed, we don't do

void f() {
#if SOMETHING
    int x;
#    if SOMETHINGELSE
    int y;
#    endif // SOMETHINGELSE
#endif // SOMETHING
}

However, what I want to avoid at all costs is unreadable crazy stuff like this:

#if defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
#if defined(__ANDROID__) && __ANDROID_API__ >= 30
#define _LIBCPP_HAS_COND_CLOCKWAIT
#elif defined(_LIBCPP_GLIBC_PREREQ)
#if _LIBCPP_GLIBC_PREREQ(2, 30)
#define _LIBCPP_HAS_COND_CLOCKWAIT
#endif
#endif
#endif

If anyone wrote normal code with ifs without indentation like that, we'd tell them it's unreadable. The same applies to the preprocessor.

To go back to this review, I think what we want is

# libc++'s preferred indentions of preprocessor statements.
IndentPPDirectives: AfterHash

That's it. We don't specify the tab size cause that depends on what file you're in.

Mordante updated this revision to Diff 374630.Sep 23 2021, 11:18 AM

Remove PPIndentWidth: 1

ldionne accepted this revision.Sep 23 2021, 12:03 PM
This revision is now accepted and ready to land.Sep 23 2021, 12:03 PM
This revision was landed with ongoing or failed builds.Sep 24 2021, 10:28 AM
This revision was automatically updated to reflect the committed changes.