[PATCH] D125604: [FileCheck] Catch missspelled directives.

[Ivan Kosarev via Phabricator via cfe-commits]

kosarev 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;
----------------
thopre wrote:
> 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?
https://discourse.llvm.org/t/help-wanted-with-fixing-misspelled-filecheck-directives/62593

Thanks for the idea!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125604