This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Move fuchsia-restrict-system-includes to portability module for general use.
ClosedPublic

Authored by PaulkaToast on Mar 6 2020, 4:24 PM.

Details

Summary

Created a general check for restrict-system-includes under portability as recommend in the comments under D75332. I also fleshed out the user facing documentation to show examples for common use-cases such as allow-list, block-list, and wild carding.

Removed fuchsia's check as per phosek sugguestion.

Diff Detail

Event Timeline

PaulkaToast created this revision.Mar 6 2020, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2020, 4:24 PM
phosek accepted this revision.Mar 6 2020, 5:17 PM

We're not using this module in Fuchsia at the moment, so I'd be fine not having an alias at all, if we start using it again in the future we'll use the new name.

This revision is now accepted and ready to land.Mar 6 2020, 5:17 PM
MaskRay added inline comments.Mar 6 2020, 5:59 PM
clang-tools-extra/docs/ReleaseNotes.rst
114

Delete fuchsia-restrict-system-includes

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
107

I don't think that this statement is necessary.

clang-tools-extra/docs/clang-tidy/checks/portability-restrict-system-includes.rst
11

Please highlight zlib.h with single back-ticks.

21

Please highlight zlib.h with single back-ticks.

51

Please use single back-ticks for highlighting options values. Same above.

PaulkaToast marked 5 inline comments as done.

Thanks for the heads up phosek, I removed the check from fuchsia's directory.
Also addressed Eurgene.Zelenko's comments. (:

Eugene.Zelenko added inline comments.Mar 6 2020, 9:55 PM
clang-tools-extra/docs/ReleaseNotes.rst
135

Please insert empty line above.

PaulkaToast marked an inline comment as done.
PaulkaToast edited the summary of this revision. (Show Details)Mar 9 2020, 3:30 PM
aaron.ballman accepted this revision.Mar 10 2020, 1:12 PM

LGTM!

clang-tools-extra/docs/clang-tidy/checks/portability-restrict-system-includes.rst
49

This is not something you have to fix (and certainly not as part of this patch), but is a note of a bug... we typically use semicolon-delimited lists, and I think that may be especially important here as comma can be a valid character in a file name on many file systems. I notice that we're using GlobList which still seems to use comma-separated values. We may want to consider allowing both semi-colon and commas in GlobList and then updating the docs to suggest semicolons instead of commas.

PaulkaToast marked 2 inline comments as done.EditedMar 10 2020, 1:35 PM
clang-tools-extra/docs/clang-tidy/checks/portability-restrict-system-includes.rst
49

thanks, I'll send a patch out for this as its own change. (:

This revision was automatically updated to reflect the committed changes.
PaulkaToast marked an inline comment as done.

@PaulkaToast This patch appears to have caused a buildbot issue, please can you investigate/revert: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869

@PaulkaToast This patch appears to have caused a buildbot issue, please can you investigate/revert: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869

I believe the issue is that we're looking for system headers that are not mocked as part of the test. Sorry about not catching that earlier in the review! @PaulkaToast, you should create an empty stdio.h (and others used by your test) in Inputs/fucscia-restrict-system-includes/system so that we're not finding the actual system includes.

@PaulkaToast This patch appears to have caused a buildbot issue, please can you investigate/revert: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869

I believe the issue is that we're looking for system headers that are not mocked as part of the test. Sorry about not catching that earlier in the review! @PaulkaToast, you should create an empty stdio.h (and others used by your test) in Inputs/fucscia-restrict-system-includes/system so that we're not finding the actual system includes.

Working on that now, I'll send out a review shortly. Thanks!

clang-tools-extra/test/clang-tidy/checkers/fuchsia-restrict-system-includes-all.cpp