This is an archive of the discontinued LLVM Phabricator instance.

Differential D90621

[FileCheck] Added documentation for --allow-unused-prefixes
ClosedPublic

Authored by mtrofin on Nov 2 2020, 8:54 AM.

Details

Reviewers
jhenderson
thopre
Commits
rG34b0a99cce81: [Docs][FileCheck] Small fix.
rG22113341d747: [FileCheck] Added documentation for --allow-unused-prefixes

Diff Detail

Unit TestsFailed

TimeTest
440 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
530 mswindows > LLVM.CodeGen/AMDGPU::ds_read2.ll
240 mswindows > lld.ELF/invalid::symtab-sh-info.s

Event Timeline

mtrofin created this revision.Nov 2 2020, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 8:54 AM
Herald added subscribers: llvm-commits, thopre. · View Herald Transcript
mtrofin requested review of this revision.Nov 2 2020, 8:54 AM
mtrofin mentioned this in D90281: [FileCheck] Report missing prefixes when more than one is provided..Nov 2 2020, 8:54 AM
Harbormaster completed remote builds in B77273: Diff 302299.Nov 2 2020, 9:42 AM
thopre added inline comments.Nov 2 2020, 9:58 AM
llvm/docs/CommandGuide/FileCheck.rst
69–70

Nit: Although this is common sense, the phrasing doesn't look like it includes the case of one --check-prefix and one --check-prefixes with a single value. How about:

This option controls the behavior when using more than one prefix as specified by :option:--check-prefix or :option:--check-prefixes and some of these prefixes are missing in the test file.

Rest is fine.

mtrofin updated this revision to Diff 302331.Nov 2 2020, 10:31 AM

feedback

mtrofin marked an inline comment as done.Nov 2 2020, 10:31 AM
Harbormaster completed remote builds in B77287: Diff 302331.Nov 2 2020, 11:06 AM
thopre accepted this revision.Nov 2 2020, 12:09 PM

LGTM, thanks!

This revision is now accepted and ready to land.Nov 2 2020, 12:09 PM
This revision was landed with ongoing or failed builds.Nov 2 2020, 12:16 PM
Closed by commit rG22113341d747: [FileCheck] Added documentation for --allow-unused-prefixes (authored by mtrofin). · Explain Why
This revision was automatically updated to reflect the committed changes.
mtrofin added a commit: rG22113341d747: [FileCheck] Added documentation for --allow-unused-prefixes.
jhenderson added inline comments.Nov 3 2020, 12:44 AM
llvm/docs/CommandGuide/FileCheck.rst
72

Nit. Feel free to directly commit that fix.

mtrofin marked an inline comment as done.Nov 3 2020, 7:10 AM
mtrofin added a commit: rG34b0a99cce81: [Docs][FileCheck] Small fix..

Revision Contents

PathSize
llvm/
docs/
CommandGuide/
10 lines

Diff 302299

llvm/docs/CommandGuide/FileCheck.rst

Show First 20 Lines • Show All 58 Lines • ▼ Show 20 Lines
These default comment prefixes can be overridden by These default comment prefixes can be overridden by
:option:`--comment-prefixes` if they are not appropriate for your testing :option:`--comment-prefixes` if they are not appropriate for your testing
environment. However, doing so is not recommended in LLVM's LIT-based test environment. However, doing so is not recommended in LLVM's LIT-based test
suites, which should be easier to maintain if they all follow a consistent suites, which should be easier to maintain if they all follow a consistent
comment style. In that case, consider proposing a change to the default comment style. In that case, consider proposing a change to the default
comment prefixes instead. comment prefixes instead.
.. option:: --allow-unused-prefixes
This option controls behavior when using more than one :option:`--check-prefix`
or the alias :option:`--check-prefixes` with multiple values, and some of these
thopreUnsubmitted
Done
ReplyInline Actions

Nit: Although this is common sense, the phrasing doesn't look like it includes the case of one --check-prefix and one --check-prefixes with a single value. How about:

This option controls the behavior when using more than one prefix as specified by :option:--check-prefix or :option:--check-prefixes and some of these prefixes are missing in the test file.

Rest is fine.

thopre: Nit: Although this is common sense, the phrasing doesn't look like it includes the case of one…
prefixes are missing in the test file. If true, this is allowed, if false,
FileCheck will report error, listing the missing prefixes.
jhendersonUnsubmitted
Done
ReplyInline Actions
prefixes are missing in the test file. If true, this is allowed, if false,
- FileCheck will report error, listing the missing prefixes.
+ FileCheck will report an error, listing the missing prefixes.
It is currently, temporarily, true by default, and will be subsequently

Nit. Feel free to directly commit that fix.

jhenderson: Nit. Feel free to directly commit that fix.
It is currently, temporarily, true by default, and will be subsequently
switched to false.
.. option:: --input-file filename .. option:: --input-file filename
File to check (defaults to stdin). File to check (defaults to stdin).
.. option:: --match-full-lines .. option:: --match-full-lines
By default, FileCheck allows matches of anywhere on a line. This By default, FileCheck allows matches of anywhere on a line. This
option will require all positive matches to cover an entire option will require all positive matches to cover an entire
▲ Show 20 Lines • Show All 853 Lines • Show Last 20 Lines