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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:51
+  /// numbers.
+  unsigned Cap : 1;
+
----------------
arichardson wrote:
> I would call this `UpperCase` or `Capitalize`. If it is only ever used for hex values (I'm not sure if there are plans for e.g. uppercasing string values later on) storing an enumeration with `HexUpper` and `HexLower`, `Unsigned` values could also make sense.
> 
> I you use the enum approach, it should also be possible to omit the Valid bitfield and use an enum value of `Implicit` or `NoFormat` instead.
> 
> I haven't managed to review the full patch yet, but maybe it is enough to store just a single enumeration value in this class that also contains the conflict value since that seems mutually exclusive with all other boolean flags.
Is there a specific reason that the 4 members must be bit fields, not regular `bool`s? I don't think that will waste space.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:104
+  /// implicit format (e.g. AST is made of a single literal).
+  virtual FileCheckExpressionFormat getImplicitFormat() const = 0;
 };
----------------
```
virtual FileCheckExpressionFormat getImplicitFormat() const {
  return FormatNone;
}
```

and delete the unnecessary override below.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:172
+  /// if any, (ii) its implicit format if any or (iii) the default format.
+  FileCheckExpressionFormat getEffectiveFormat() { return Format; }
+};
----------------
Why isn't this `getFormat() const`? Or rename the member to `EffectiveFormat;`.

I think it is probably clearer to merge the comment with `FileCheckExpressionFormat Format;` and delete the comment here. For a class with 2 getters, I think the definition is probably too long.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:212
 
+  /// \returns expression associated with this numeric variable.
+  FileCheckExpression *getExpression() const { return Expression; }
----------------
This comment is redundant. I know you added some to other getters in previous patches, but I think they can also be deleted.


================
Comment at: llvm/lib/Support/FileCheck.cpp:29
+operator==(const FileCheckExpressionFormat &other) {
+  return Valid == other.Valid && Hex == other.Hex && Cap == other.Cap;
+}
----------------
Consider defining this inline. The definition is trivial and `FileCheck.h` is only included in two `.cpp` files.


================
Comment at: llvm/lib/Support/FileCheck.cpp:54
+
+  if (StrVal.getAsInteger(Radix, Value))
+    return FileCheckErrorDiagnostic::get(SM, StrVal,
----------------
I think `if (StrVal.getAsInteger(Hex ? 16 : 10, Value))` is just as clear. (Or `(Hex ? 16 : 10)`. The empty line above can also be deleted.


================
Comment at: llvm/lib/Support/FileCheck.cpp:131
+  FileCheckExpressionFormat Format =
+      (LeftFormat.Valid) ? LeftFormat : RightFormat;
+  if (LeftFormat.Valid && RightFormat.Valid && LeftFormat != RightFormat)
----------------
`LeftFormat.Valid ? LeftFormat : RightFormat;`


================
Comment at: llvm/lib/Support/FileCheck.cpp:194
+// true if string SkipStr was not in S. Returns false otherwise.
+static bool stripFront(StringRef &S, const StringRef &SkipStr) {
+  return !S.consume_front(SkipStr);
----------------
Delete the helper and inline it into the call sites.


================
Comment at: llvm/lib/Support/FileCheck.cpp:287
   uint64_t LiteralValue;
-  if (!Expr.consumeInteger(/*Radix=*/10, LiteralValue))
+  if (!Expr.consumeInteger(Radix, LiteralValue))
     return std::make_shared<FileCheckExpressionLiteral>(LiteralValue);
----------------
I think `if (!Expr.consumeInteger(AO == Literal ? 10 : 0, LiteralValue))` is just as clear as the current one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389