Page MenuHomePhabricator

[FileCheck] Suppress old -v/-vv diags if dumping input

Authored by jdenny on Dec 18 2018, 7:04 AM.



The old diagnostic form of the trace produced by -v and -vv looks

check1:1:8: remark: CHECK: expected string found in input
CHECK: abc 
<stdin>:1:3: note: found here
; abc def 

When dumping annotated input is requested (via -dump-input), I find
that this old trace is not useful and is sometimes harmful:

  1. The old trace is mostly redundant because the same basic information also appears in the input dump's annotations.
  1. The old trace buries any error diagnostic between it and the input dump, but I find it useful to see any error diagnostic up front.
  1. FILECHECK_OPTS=-dump-input=fail requests annotated input dumps only for failed FileCheck calls. However, I have to also add -v or -vv to get a full set of annotations, and that can produce massive output from all FileCheck calls in all tests. That's a real problem when I run this in the IDE I use, which grinds to a halt as it tries to capture all that output.

When -dump-input=fail|always, this patch suppresses the old trace from
-v or -vv. Error diagnostics still print as usual. If you want the
old trace, perhaps to see variable expansions, you can set
-dump-input=none (the default).

Diff Detail


Event Timeline

jdenny created this revision.Dec 18 2018, 7:04 AM

Thanks for the ping. The code change looks straightforward, but a handful of questions on the tests.

24 ↗(On Diff #178666)

This kind of sequence always makes me wonder whether the NOT text might appear after one of the other lines gets a match. If 'remark' shouldn't occur at all, then maybe use -implicit-check-not on the run line?

62 ↗(On Diff #178666)


97 ↗(On Diff #178666)


132 ↗(On Diff #178666)


183 ↗(On Diff #178666)


202 ↗(On Diff #178666)


244 ↗(On Diff #178666)


276 ↗(On Diff #178666)


309 ↗(On Diff #178666)


337 ↗(On Diff #178666)


382 ↗(On Diff #178666)


427 ↗(On Diff #178666)


7 ↗(On Diff #178666)

So, this first set of changes goes from matching (or not) on the first line, to always matching the first line, and matching (or not) on the second line. Does that make a material difference in the test?

46 ↗(On Diff #178666)

Offhand it looks like every use of -dump-input in this file now also has -v which seems excessive. Also like you're losing coverage of that combination. Think about using -v more judiciously here.

jdenny marked 3 inline comments as done.Jan 8 2019, 3:35 PM

Thanks for reviewing!

24 ↗(On Diff #178666)

Currently, the remarks forming the trace all come before the error because FileCheck stops at the error, but that could change in the future, intentionally or not. Good idea. I'll adjust.

7 ↗(On Diff #178666)

With that change, -v (when -dump-input=none) always produces a remark for the trace so we can always test whether the trace is suppressed.

Without that change, there's only a remark for the trace if the first line matches.

46 ↗(On Diff #178666)

Offhand it looks like every use of -dump-input in this file now also has -v which seems excessive.

The purpose of this test file is to check the effects of various ways of disabling/enabling input dumps. Checking for a trace seems important because disabling/enabling input dumps in any manner is now intended to permit/suppress the trace. I don't see a reason to skip that check for just some ways of disabling/enabling input dumps.

Also like you're losing coverage of that combination.

Combinations without -v? I feel like related dimensions of functionality are tested well elsewhere, such as verbose.txt and dump-input-annotations.txt.

Think about using -v more judiciously here.

If the above explanation of my testing strategy isn't satisfying, please explain your objection a little more. Maybe I'm misunderstanding.

probinson added inline comments.Jan 9 2019, 7:25 AM
46 ↗(On Diff #178666)

verbose.txt looks at -v/-vv without -dump-input. dump-input-annotations.txt uses only -dump-input=always and not the other flavors. The behavior of -dump-input=never/fail without -v is not verified anywhere that I can find.

jdenny added inline comments.Jan 9 2019, 11:15 AM
46 ↗(On Diff #178666)

Ah, I see. I don't want to lose coverage of trace suppression by -dump-input, so I'll add a few new checks here for the cases you've mentioned. Thanks.

jdenny updated this revision to Diff 180945.Jan 9 2019, 3:30 PM
jdenny marked 16 inline comments as done.
  • In dump-input-annotations.txt, use -implicit-check-not to thoroughly check for trace suppression.
  • In dump-input-enable.txt, improve comments, and cover cases of -dump-input=never|fail without -v.
probinson accepted this revision.Jan 14 2019, 1:42 PM

LGTM although I do wonder at the usefulness of the wildcards in the implicit-not checks.

20 ↗(On Diff #180945)

Is this different from -implicit-check-not='remark:' ? I wouldn't have thought so. I mean, in a positive CHECK, you use the wildcards to hoover up the rest of the line, but in a NOT it doesn't seem useful.

This revision is now accepted and ready to land.Jan 14 2019, 1:42 PM
jdenny marked an inline comment as done.Jan 14 2019, 7:35 PM
jdenny added inline comments.
20 ↗(On Diff #180945)

Ah, I hadn't noticed that -match-full-lines only affects positive matches. I'll adjust all those before committing.

Thanks for the review!

This revision was automatically updated to reflect the committed changes.