[PATCH] D60389: FileCheck [9/12]: Add support for matching formats

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not looked at the tests in the latest version, but I've made a number of style and comment comments for you.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:617
+  is inferred from the matching format of the numeric variable(s) used by the
+  expression constraint if any, and default to ``%u`` if no numeric variable is
+  used.  In case of conflict between matching formats of several numeric
----------------
default -> defaults


================
Comment at: llvm/lib/Support/FileCheck.cpp:30-32
+  default:
+    assert("Trying to match value with invalid format");
+    return StringRef();
----------------
Probably the more canonical way of writing these sort of switch statements is:
```
switch (Value) {
case 1:
...
case 2:
...
default:
  llvm_unreachable("unknown format value");
}
```

(If the code isn't truly unreachable on the other hand, it should be an error, not an assertion)


================
Comment at: llvm/lib/Support/FileCheck.cpp:98
+
+  ExpressionFormat Format = LeftFormat ? LeftFormat : RightFormat;
+  if (LeftFormat && RightFormat && LeftFormat != RightFormat)
----------------
I don't know about anybody else, but `LeftFormat != NoFormat ? LeftFormat : RightFormat` is more readable to me (and removes the need to hard-code the enum value).


================
Comment at: llvm/lib/Support/FileCheckImpl.h:33-34
 
+/// Bitfield representing the format an expression value should be printed into
+/// for matching. Used to represent both explicit format specifiers as well as
+/// implicit format from using numeric variables.
----------------
The comments in this class and its enum need updating following the recent changes to it.

Also, "printed into for matching" doesn't really make sense. Probably something like "converted into" would make more sense.


================
Comment at: llvm/lib/Support/FileCheckImpl.h:42
+    NoFormat = 0,
+    /// there are several conflicting implicit formats in an expression.
+    Conflict,
----------------
Perhaps change this to "Used when there are..."


================
Comment at: llvm/lib/Support/FileCheckImpl.h:68
+  ExpressionFormat() : Value(FormatValue::NoFormat){};
+  ExpressionFormat(FormatValue Value) : Value(Value){};
+
----------------
`explicit`?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:95-96
+
+  /// \returns implicit format of this AST, FormatConflict if implicit formats
+  /// of the AST's components conflict and NoFormat if the AST has no implicit
+  /// format (e.g. AST is made of a single literal).
----------------
either implicit format of this AST ... or NoFormat if the AST has...


================
Comment at: llvm/lib/Support/FileCheckImpl.h:97
+  /// of the AST's components conflict and NoFormat if the AST has no implicit
+  /// format (e.g. AST is made of a single literal).
+  virtual ExpressionFormat getImplicitFormat() const {
----------------
made -> made up


================
Comment at: llvm/lib/Support/FileCheckImpl.h:171
 
+  /// Format to use for expressions using this variable without explicit format
+  /// and without conflict with another variable.
----------------
"without an explicit"

I don't think you need the rest of the sentence after that: the concept of a conflict isn't important to the variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389