[PATCH] D81667: [RFC, FileCheck] Add precision to format specifier

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:86
   }
 }
 
----------------
thopre wrote:
> grimar wrote:
> > Perhaps, it might be simpler just to merge switches and write the logic here as:
> > 
> > 
> > ```
> > Expected<std::string> ExpressionFormat::getWildcardRegex() const {
> >   if (Value == Kind::NoFormat)
> >     return createStringError(std::errc::invalid_argument,
> >                              "trying to match value with invalid format");
> >   switch (Value) {
> >   case Kind::Unsigned:
> >     if (Precision)
> >       return ("-?([1-9][0-9]*)?[0-9]{" + Twine(Precision) + "}").str();
> >     return std::string("[0-9]+");
> >   case Kind::Signed:
> >      ...
> >   default:
> >     llvm_unreachable("....");
> >   }
> > }
> > ```
> I'm not a big fan of repeating the formatting logic for the Precision case so I've kept that bit as is. What do you think of the result?
I see 2 possible improvements:

1) When you have a dedicated `RegexPrefix` variable, you postpone the return and have to add `break`s everywhere.
    If you just do not want to repeat the formatting logic, I'd suggest to add a little helper. E.g:


```
  auto CreatePrecisionRegex = [](StringRef S) -> std::string {
    return (S + Twine(Precision) + "}").str();
  };

  switch (Value) {
  case Kind::Unsigned:
    if (Precision)
      return CreatePrecisionRegex("-?([1-9][0-9]*)?[0-9]{");
    return std::string("[0-9]+");
  default:
    llvm_unreachable("ddd");
  }
```

The main benefit is that you can return early and avoid having a one more variable.

2) Perhaps it doesn't make much sence to use `createStringError` for the `default` case? It is unreachable now and can't be tested either (I believe). 

So I'd either remove the `if (Value == Kind::NoFormat)` block and handle the error in the `default`, like you initially did,
or keep it and switch to using `llvm_unreachable` in `default`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81667