This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Normalize all our '#pragma GCC system_header', and regression-test.
ClosedPublic

Authored by Quuxplusone on Feb 2 2022, 8:07 AM.

Details

Summary

Now we'll notice if a header forgets to include this magic phrase.

(I'm sympathetic to the argument that we should add a new linter test for each different check, instead of piggybacking every check into lint_headers.py (which we could rename to indicate that it checks nothing but include-ordering). However, I don't think we've reached the tipping point of complexity yet. :))

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 2 2022, 8:07 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 2 2022, 8:07 AM
jloser accepted this revision.Feb 2 2022, 8:33 AM
jloser added inline comments.
libcxx/test/libcxx/lint/lint_headers.sh.py
46

Nit: I think we rely on python3 exclusively now, so we could make this a Python3 f-string which I think is nicer. Thoughts? I hear the argument that this is consistent with the rest of the file (because it was mostly written when we only supported Python2).

Quuxplusone added inline comments.Feb 2 2022, 9:15 AM
libcxx/test/libcxx/lint/lint_headers.sh.py
46

I hear the argument that this is consistent with the rest of the file (because it was mostly written when we only supported Python2).

Actually it was written very recently, but by an old curmudgeon who likes printf. Between these options

  • print('foo/%s!' % pretty(x))
  • print('foo/{}!'.format(pretty(x)))
  • print(f'foo/{pretty(x)}!')

I'm personally inclined to favor #1, just for readability's sake... although I suspect I'm on the losing side of history on this one. ;)

jloser added inline comments.Feb 2 2022, 9:19 AM
libcxx/test/libcxx/lint/lint_headers.sh.py
46

Ah yes, how could I forget it was written by you last month? :)

I prefer option 3 with the f-string, but maybe I'm just a new-school Python guy. If we care to update these kinds of things, we can use some tool (maybe Black or Ale or something) to fix (and maybe even enforce if we care) "modern" use, so don't feel obligated to do anything in this PR.

ldionne requested changes to this revision.Feb 2 2022, 9:41 AM

This LGTM, but I care strongly that we indent nested preprocessor directives. This is not different from indenting normal if-else chains.

In a 3-line #if, this is not so bad, however when we start having complicated nested conditions like we do in __config, it's basically impossible to read unless things are indented properly.

This revision now requires changes to proceed.Feb 2 2022, 9:41 AM
Mordante requested changes to this revision.Feb 2 2022, 9:43 AM

Let's first agree on how the we want to format #pragma GCC system_header\n.

libcxx/include/__format/formatter_pointer.h
27 ↗(On Diff #405287)

The spaces in the #if is something we recently changed in the .clang-format file on @ldionne's request https://reviews.llvm.org/D103368#inline-1043939. So I would prefer to keep it that way. If we agree to remove the spaces, please also update the .clang-format. I know we don't use clang-format, but I've been using it in the formatter code since its inception and I prefer to keep using it.

libcxx/include/__format/formatter_pointer.h
27 ↗(On Diff #405287)

@ldionne, do you want me to change just these few files, or do you want me to change all the rest of the files? I can go either way; it's a simple sed command to change all of them. Personally I'd rather touch the fewest files, because to me the extra couple of whitespaces cost 2 bytes per file and buy literally nothing; but I can sed them all if we want.

What I won't do in this PR is introduce inconsistency. For example, I'm not going to start adding # pragma directives to some files while other files still contain ordinary #pragma directives.

Mordante added inline comments.Feb 2 2022, 10:48 AM
libcxx/include/__format/formatter_pointer.h
27 ↗(On Diff #405287)

I agree, I too don't want inconsistencies! When we want to remove the spaces, then lets also revert the .clang-format patch that adds these spaces. I don't have a strong opinion on whether or not to add spaces.

ldionne accepted this revision.Feb 2 2022, 3:09 PM

LGTM with spaces consistently on all pragmas.

libcxx/include/__format/formatter_pointer.h
27 ↗(On Diff #405287)

Let's add the spaces in this PR too.

because to me the extra couple of whitespaces cost 2 bytes per file and buy literally nothing

Do you indent your ifs? Of course, you do. Why?

git grep -l 'pragma GCC system_header' ../libcxx/include/ | xargs sed -i -e 's/#pragma GCC system_header/# pragma GCC system_header/'

@ldionne @Mordante LGTY?

Mordante accepted this revision.Feb 3 2022, 11:16 PM

Thanks for the cleanup, LGTM.

libcxx/test/libcxx/lint/lint_headers.sh.py
46

I suggest this change to match the search string.

This revision is now accepted and ready to land.Feb 3 2022, 11:16 PM
This revision was landed with ongoing or failed builds.Feb 4 2022, 9:28 AM
This revision was automatically updated to reflect the committed changes.