[PATCH] D55940: Detect incorrect FileCheck variable CLI definition

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

jdenny added a comment.

Other than nits I've added as inline comments, LGTM.  However, let's give Paul some time to respond now that the holidays are over.



================
Comment at: llvm/lib/Support/CommandLine.cpp:436
+  auto O = I->second;
+  if (O->getFormattingFlag() == cl::AlwaysPrefix)
+    return nullptr;
----------------
Is it ever possible for `O` to be nullptr here?


================
Comment at: llvm/test/FileCheck/defines.txt:8
+; RUN: not FileCheck -D=10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIVAR
+; RUN: FileCheck -DVALUE= -check-prefix EMPTY -input-file %s %s 2>&1
 
----------------
I suggest testing `-D=` too.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:531
+    if (EqIdx == std::string::npos) {
+      errs() << "Missing equal sign in command-line definition '" << G << "'\n";
+      GlobalDefineError = true;
----------------
I think these two diagnostics would be clearer mentioning `-D`.  For example:

```
errs() << "Missing equal sign in command-line definition: -D" << G << "\n";
```


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:534
+      continue;
+    } else if (EqIdx == 0) {
+      errs() << "Missing pattern variable name in command-line definition '"
----------------
LLVM coding standards say not to use `else if` after `continue`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55940