[FileCheck] Permit multiple -v or -vv

[jdenny]

Summary

FILECHECK_OPTS was implemented so that a test runner, such as CI,
can specify FileCheck debugging options, such as -v and -vv.
However, if a test suite has a FileCheck call that already specifies
-v or -vv, then that call will fail if FILECHECK_OPTS also
specifies it.

For -vv, this problem already exists:

clang/test/CodeGen/aarch64-v8.2a-fp16-intrinsics-constrained.c

It's not yet clear if the -vv in that test was intentional, but this
usage shouldn't fail anyway. It's already true that FileCheck permits
-vv and -v together even though -vv implies -v.

Compare D70784, which fixed the same problem for -dump-input.

[jhenderson]

Tentative LGTM, but I'd like to see the test tidy-up separated out before formally approving.

llvm/test/FileCheck/verbose.txt

17

More than happy for you to make these tidy ups, but can you do them in a separate change, please? It makes it a little trickier to identify which cases existed before, versus which ones are new.

[thopre]

LGTM with James' suggestion

[jdenny]

Changed RUN style to match what I now recall has been recommended in the past: | at end not beginning of line.

Extracted style changes of existing RUN lines to a parent patch, as requested.

[thopre]

LGTM, thanks. Strange to have the pipe at the end but if that's the agreed upon practice so be it.

[jdenny]

@jhenderson, @thopre Thanks for the quick reviews!

Strange to have the pipe at the end but if that's the agreed upon practice so be it.

Well, a reviewer requested it at least once. @jhenderson, wasn't that you? Should we come to a broader consensus about this style, at least for FileCheck's test suite?

I don't think that point should delay these patches, but I'd be happy to go back and revise if we arrive at a different conclusion.

[jhenderson]

In D82601#2117151, @jdenny wrote:

@jhenderson, @thopre Thanks for the quick reviews!

Strange to have the pipe at the end but if that's the agreed upon practice so be it.

Well, a reviewer requested it at least once. @jhenderson, wasn't that you? Should we come to a broader consensus about this style, at least for FileCheck's test suite?

I don't think that point should delay these patches, but I'd be happy to go back and revise if we arrive at a different conclusion.

I personally recommend that style these days as a slight readability improvement. I don't think there's any particular widespread agreement on what is the best way though, and wouldn't ask for mass changes in existing tests. The idea is that from the line with the pipe on, you can tell that the complete command is written on that line, and that the next line will be a continuation, whilst the next line's extra 2 spaces of indentation show that it's a continuation from the previous line (and since it starts with a command is the start of that command). The alternative approach of the pipe being on the next line requires people to look at the next line to confirm whether or not all the command's arguments have been included.

LGTM.

[jdenny]

In D82601#2119199, @jhenderson wrote:

I personally recommend that style these days as a slight readability improvement. I don't think there's any particular widespread agreement on what is the best way though, and wouldn't ask for mass changes in existing tests. The idea is that from the line with the pipe on, you can tell that the complete command is written on that line, and that the next line will be a continuation, whilst the next line's extra 2 spaces of indentation show that it's a continuation from the previous line (and since it starts with a command is the start of that command). The alternative approach of the pipe being on the next line requires people to look at the next line to confirm whether or not all the command's arguments have been included.

Thanks for the reminder. At the very least, this style seems consistent with other styles I've seen in LLVM. For example, before LLVM, I was used to the following style for operators in C/C++:

if (very_long_cond_1
    && very_long_cond2)
  statement;

But LLVM's style is:

if (very_long_cond_1 &&
    very_long_cond_2)
  statement;

llvm/test/FileCheck/verbose.txt

 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck --dump-input=never --input-file %s %s 2>&1 | \
 ; RUN:   FileCheck --check-prefix QUIET --allow-empty %s
 
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck --dump-input=never -v --input-file %s %s 2>&1 | \
 ; RUN:   FileCheck --strict-whitespace --check-prefix V %s
 
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck --dump-input=never -vv --input-file %s %s 2>&1 | \
 ; RUN:   FileCheck --strict-whitespace --check-prefixes V,VV %s
 
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck --dump-input=never -v -v --input-file %s %s 2>&1 | \
+; RUN:   FileCheck --strict-whitespace --check-prefix V %s
+
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck --dump-input=never -vv -vv --input-file %s %s 2>&1 | \
+; RUN:   FileCheck --strict-whitespace --check-prefixes V,VV %s
+
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck --dump-input=never -v -vv --input-file %s %s 2>&1 | \
+; RUN:   FileCheck --strict-whitespace --check-prefixes V,VV %s
+
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck --dump-input=never -vv -v --input-file %s %s 2>&1 | \
+; RUN:   FileCheck --strict-whitespace --check-prefixes V,VV %s
+
 ; END.
 
 foo
 bar
 CHECK: foo
 CHECK-NOT: raboof
 CHECK-NEXT: bar
 

llvm/utils/FileCheck/FileCheck.cpp

 static cl::opt<bool> AllowDeprecatedDagOverlap(
     "allow-deprecated-dag-overlap", cl::init(false),
     cl::desc("Enable overlapping among matches in a group of consecutive\n"
              "CHECK-DAG directives.  This option is deprecated and is only\n"
              "provided for convenience as old tests are migrated to the new\n"
              "non-overlapping CHECK-DAG implementation.\n"));
 
 static cl::opt<bool> Verbose(
-    "v", cl::init(false),
+    "v", cl::init(false), cl::ZeroOrMore,
     cl::desc("Print directive pattern matches, or add them to the input dump\n"
              "if enabled.\n"));
 
 static cl::opt<bool> VerboseVerbose(
-    "vv", cl::init(false),
+    "vv", cl::init(false), cl::ZeroOrMore,
     cl::desc("Print information helpful in diagnosing internal FileCheck\n"
              "issues, or add it to the input dump if enabled.  Implies\n"
              "-v.\n"));
 
 // The order of DumpInputValue members affects their precedence, as documented
 // for -dump-input below.
 enum DumpInputValue {
   DumpInputNever,