thopre added inline comments.
================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1774-1781
+static std::pair<Check::FileCheckType, StringRef>
+FindCheckType(const FileCheckRequest &Req, StringRef Buffer, StringRef Prefix)
{
+ bool Misspelled = false;
+ auto Res = FindCheckType(Req, Buffer, Prefix, Misspelled);
+ if (Res.first != Check::CheckNone && Misspelled)
+ return {Check::CheckMisspelled, Res.second};
+ return Res;
----------------
kosarev wrote:
> thopre wrote:
> > kosarev wrote:
> > > thopre wrote:
> > > > Instead of introducing a new wrapper, why don't you change all the
> > > > return to call a constructor method (e.g. `make_check_type()`) that
> > > > does what this wrapper do? Then there would not be any FindCheckType
> > > > that take a Misspelled parameter.
> > > >
> > > > I'm also not sure about Misspelled being a check kind. It feels
> > > > conceptually wrong but on the other hand I guess it makes the
> > > > implementation simpler.
> > > Tried that. Replacing the returned pair with a new `CheckLine` kind of
> > > object implementing the misspelled-related logic seems to add a lot of
> > > extra clutter such as the definition of the new structure itself, but
> > > especially all the repetitive mentions of `Misspelled` on every `return`.
> > > Feels like having it as a reference parameter works better, as we only
> > > need to alter the flag occasionally.
> > >
> > > Regarding `CheckMisspelled`, now that we have `CheckBadNot` and
> > > `CheckBadCount`, this looks the usual way of propagating the information
> > > about our spelling-related concerns. Might be not the best design and may
> > > be worth looking into at some point, but at least doesn' seem to be
> > > specific to this patch?
> > I was thinking something along the line of:
> >
> > return getRealCheckType(CHECK::CheckBadCount, Rest, Misspelled); with:
> >
> > ```static std::pair<Check::FileCheckType, StringRef>
> > getRealCheckType(Check::FileCheckType CheckType, StringRef Rest, bool
> > Misspelled) {
> > if (CheckType != Check::CheckNone && Misspelled)
> > return {Check::CheckMisspelled, Rest};
> > return {CheckType, Rest};
> > }```
> >
> > Fair enough for CheckMisspelled, there is indeeed precedent.
> That unfortunately wouldn't eliminate the repetitive `return
> getRealCheckType(..., Misspelled)` bits, thus adding a significant amount of
> clutter -- all for the sake of a single assignment where we raise the flag,
> while also making the code more fragile as the compiler wouldn't then be able
> to catch `return`s without calling `getRealCheckType()`. And if that's not
> enough, then the name of the function sounds like we introduce one of the
> most irritating kinds of concepts -- the 'real' ones. :-)
Fair enough. LGTM for the FileCheck part then. Have you sent a message to
discourse to ask test authors for help on the TODO?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125604/new/
https://reviews.llvm.org/D125604
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
