[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
   }
 }
 
----------------
grimar wrote:
> 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`.
Oh, and for `1)` there is no need to use `-> std::string`:


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

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81667