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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny marked 3 inline comments as done.
jdenny added a comment.

Thanks for reviewing!



================
Comment at: llvm/test/FileCheck/dump-input-annotations.txt:24
+; ALIGN:{{.*}}error:{{.*}}
+; ALIGN:{{.*}}possible intended match here{{.*}}
+
----------------
probinson wrote:
> 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?
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.



================
Comment at: llvm/test/FileCheck/dump-input-enable.txt:7
 
-; RUN: echo 'CHECK: ciao' > %t.check
+; RUN: echo 'CHECK: hello' > %t.check
 ; RUN: echo 'CHECK-NEXT: world' >> %t.check
----------------
probinson wrote:
> 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? 
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.


================
Comment at: llvm/test/FileCheck/dump-input-enable.txt:46
+; RUN:           -match-full-lines -dump-input=never -v 2>&1 \
+; RUN: | FileCheck %s -match-full-lines -check-prefixes=TRACE,NODUMP
 
----------------
probinson wrote:
> 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.
> 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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55825/new/

https://reviews.llvm.org/D55825