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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:86
   }
 }
 
----------------
grimar wrote:
> 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();
> };
> 
> ```
Ah yes, I started doing it your way and changed in the middle. I'll remove the top if block


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81667