[PATCH] D97845: [FileCheck] Add support for hex alternate form in FileCheck

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

jdenny added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:779
+* ``#`` is an optional flag available for hex values (see
+  ``<conversion specifier>`` below) which require the value matched to be
+  prefixed by ``0x``.
----------------
The subject is "flag" not "values".


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:131
   bool ValueIsSigned = Value == Kind::Signed;
   StringRef OverflowErrorStr = "unable to represent numeric value";
+
----------------
Based on the comments below, it seems the name `OverflowErrorStr` is misleading.  It's a general integer parse error.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:148-150
+  // Error out for prefix only if failing to parse integer because that
+  // indicates a more serious issue with the input. It also avoid giving a
+  // prefix error for e.g. -0x18.
----------------
Does this edit say what you mean?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:152
+  if (MissingFormPrefix)
+    return ErrorDiagnostic::get(SM, StrVal, "missing alternate form prefix");
+
----------------
jdenny wrote:
> Is there a test for this diagnostic?
Thanks for the unit test.

Is this error possible within the normal FileCheck utility?  That is, if the regex matched, then the match must have the prefix, right?


================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:57
   unsigned Precision = 0;
+  bool AlternateForm = false;
 
----------------
thopre wrote:
> jdenny wrote:
> > Can we have a more exact name than `AlternateForm`?  Maybe `HexPrefix`.  If generality is better, then maybe `TypePrefix`.
> That is taken from printf's manual:
> 
> 
> 
> >        The character % is followed by zero or more of the following flags:
> > 
> >        #      The value should be converted to an "alternate form".
> 
> For now it only applies to hex values but if we ever support one of the other format e.g. float format) then it would make sense to make it generic. I could rename it for now, we'll need to remember to rename it if a floating-point format gets supported at some point
> 
Ah, thanks, seems like the right name then.  Maybe we can have a comment, such as:

```
/// Is a printf-like "alternate form" specified?
```


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:263
+    LongNumberStr = addBasePrefix(RefusedHexOnlyDigits);
     checkWildcardRegexCharMatchFailure(RefusedHexOnlyDigits);
   }
----------------



================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:266
+  LongNumberStr = addBasePrefix(FirstInvalidCharDigits);
   checkWildcardRegexCharMatchFailure(FirstInvalidCharDigits);
 
----------------



================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:1095
+  expectDiagnosticError("alternate form only supported for hex values",
+                        Tester.parseSubst("%#d, PREFIXED_UNSI:").takeError());
+
----------------
How do you decide what cases to test here for the FileCheck library but not for the FileCheck utility?  For example, I didn't find these cases tested for the utility.  I'm not saying that's wrong.  I'm just trying to figure out what the right approach is.  I feel like we've discussed this before, but I've forgotten.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97845