jdenny marked an inline comment as done.
jdenny added inline comments.
================
Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377
+ // We do not allow using -implicit-check-not when an explicitly specified
+ // check prefix is not present in the input buffer.
+ if ((Req.IsDefaultCheckPrefix || FoundUsedPrefix) && !DagNotMatches.empty())
{
----------------
grimar wrote:
> jdenny wrote:
> > grimar wrote:
> > > jdenny wrote:
> > > > grimar wrote:
> > > > > jdenny wrote:
> > > > > > I find this logic confusing. Its goal appears to be to constrain
> > > > > > when `DagNotMatches` are added to `CheckStrings`. However, the
> > > > > > real goal is to constrain when FileCheck complains that there are
> > > > > > no used prefixes. Would the following logic work?
> > > > > >
> > > > > > ```
> > > > > > if (!FoundUsedPrefix && (ImplicitNegativeChecks.empty() ||
> > > > > > !Req.IsDefaultCheckPrefix)) {
> > > > > > errs() << "error: no check strings found with prefix"
> > > > > > . . .
> > > > > > }
> > > > > > if (!DagNotMatches.empty()) {
> > > > > > CheckStrings->emplace_back(
> > > > > > . . .
> > > > > > }
> > > > > > ```
> > > > > This looks better and works, thanks!
> > > > >
> > > > > (The code above has `DagNotMatches = ImplicitNegativeChecks;` line
> > > > > which is also a bit confusing probably)
> > > > > This looks better and works, thanks!
> > > >
> > > > Thanks for making that change.
> > > >
> > > > > (The code above has DagNotMatches = ImplicitNegativeChecks; line
> > > > > which is also a bit confusing probably)
> > > >
> > > > I'm fine with that one. DagNotMatches starts out as
> > > > ImplicitNegativeChecks, and CHECK-NOTs might be added later.
> > > >
> > > > I think the second sentence in the following comment still reflects the
> > > > old logic:
> > > >
> > > > ```
> > > > // When there are no used prefixes we report an error.
> > > > // We do not allow using -implicit-check-not when an explicitly
> > > > specified
> > > > // check prefix is not present in the input buffer.
> > > > ```
> > > >
> > > > Something like the following seems better to me:
> > > >
> > > > ```
> > > > // When there are no used prefixes we report an error except in the
> > > > case that
> > > > // no prefix is specified explicitly but -implicit-check-not is
> > > > specified.
> > > > ```
> > > >
> > > > the real goal is to constrain when FileCheck complains that there are
> > > > no used prefixes
> > >
> > > btw, do you know why FileCheck seems intentionally allows the case when
> > > `--check-prefixes=KNOWN,UNKNOWN`?
> > > I've mentioned in the description of D78110 that there are about 1000
> > > tests that have this. but is it a feature or a something that we might
> > > want to fix?
> > There was a long discussion about diagnostics like that over a year ago. I
> > believe we described that case as prefixes that are defined but not used.
> >
> > In some of my downstream work, I often create FileCheck prefix schemes
> > reflecting various combinations of features I want to test. For example:
> > FOO, BAR, FOO-AND-BAR, FOO-NOT-BAR, etc. It's far easier to maintain those
> > tests if I list all appropriate prefixes for each FileCheck command even if
> > some prefixes are not currently used in the test file. I don't know if
> > this use case exists upstream right now.
> >
> > Also, when developing or debugging tests involving many prefixes and many
> > FileCheck commands, I think it could be painful to have to update all
> > FileCheck commands every time you temporarily remove the only uses of some
> > prefix.
> >
> > If you pursue this diagnostic, please make it optional, even if on by
> > default. At one time, we discussed a warning system for such diagnostics.
> > Warnings could be configurable via command-line options, possibly passed
> > via the FILECHECK_OPTS env var so that it's easy to relax them during
> > debugging or based on a test suite's specific needs.
> >
> > @probinson and @SjoerdMeijer might want to chime in as they were also part
> > of previous discussions.
> I see, thanks for details.
>
> D78110 and this patch (which revealed rG09331fd7) showed that we had test
> cases committed which
> had unknown prefixes placed by mistake and hence false positives.
> I remember how accidentally pushed on review the patch with the same issue
> and nobody noticed..
> I just think that making FileCheck stricter could prevent such issues.
>
> If we'll have a consensus about making `--check-prefixes` stricter I'll be
> happy to work on this.
As long as it's optional, I think I'd be fine with that diagnostic, and I agree
it would probably be more helpful than harmful in most cases. Of course,
check-all would need to be run before pushing to be sure.
Instead of the system of warning options I mentioned, we might just have
something simple like `-allow-unused-prefix` (similar to `-allow-empty`) to
disable it when it gets in the way.
To me, "unknown prefix" is ambiguous and sounds more like "undefined prefix",
which I interpret to mean "never specified in a `-prefix[es]`". I prefer
"unused prefix" for this diagnostic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78024/new/
https://reviews.llvm.org/D78024
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
