On Wed, 2023-09-06 at 15:50 +0200, Benjamin Priour wrote:
> Hi David,
> Thanks for the review.
>
>
>
> On Tue, Sep 5, 2023 at 1:53 PM David Malcolm <dmalc...@redhat.com>
> wrote:
>
> > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote:
> >
> >
> [...snip...]
>
>
> > All of these "new" tests (apart from the "-noexcept" ones) look
> > like
> > they're meant to be existing tests that were moved, but where the
> > copy
> > of the test in gcc.dg/analyzer didn't get deleted, so they show up
> > as a
> > duplicate. See the details below.
> >
>
> > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test.
> >
> > When duplicating a test like this, the test isn't entirely "new",
> > so
> > please say something like this in the ChangeLog entry, to make it
> > clear
> > where it came from:
> >
> >
> I actually wasn't sure about these -noexcept tests. They were part
> of gcc.dg/analyzer, thus only gcc was running them. Exceptions
> were not disabled *explicitly*, but since it was C, they weren't
> enabled
> either.
>
> Therefore, the -noexcept tests are basically a copy, but with an
> explicit
> -fno-exceptions specification.
> When I duplicated them in that way I was thinking about making it
> clear
> that these tests fail in C++ with exceptions enabled, so that we
> would
> already
> have easy-to-spot failing tests to challenge a future exception
> support.
Ah, OK; let's go with your approach.
>
> Though perhaps *not* duplicating the tests but rather simply specify
> -fno-exceptions,
> with a comment "Fails with exceptions" may be better.
[...snip...]
> > > @@ -45,7 +45,7 @@ void test(int n)
> > > struct iter *it = iter_new (0, n, 1);
> > > while (!iter_done_p (it))
> > > {
> > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE"
> > > "true" {
> > xfail *-*-* } } */
> > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE"
> > > "true" } */
> > > /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */
> > > /* TODO(xfail^^^): ideally we ought to figure out i > 0
> > > after 1st
> > iteration. */
> > >
> >
> > Presumably due to the change to
> > region_model::add_constraints_from_binop, right?
> > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN
> > here, or
> > can that line be removed?
> > If so, then the 3rd comment can presumably become:
> >
> >
> The bogus here still make sense - without it there is an excess error
> -.
> I had checked for it because I too thought it could be removed.
> If I remember it correctly, we get UNKNOWN during the widening pass.
(nods)
>
>
> > > /* TODO: ideally we ought to figure out i > 0 after 1st
> > iteration. */
> >
> > [...snip...]
> >
> >
> >
> [...snip...]
>
> Thanks for spotting the files I forgot to remove from gcc.dg.
> Sorry about them, I had messed up my test folder when checking for
> arm-eabi,
> and I apparently missed some duplicates when retrieving my save.
>
> As for the files the likes of inlining-*.c, i.e. noted as Moved
> to..../...here.
> at the end of the ChangeLog, some tests checking for multiline
> outputs
> are so heavily rewritten than git marks them as Removed/New test
> instead of moved. I've manually edited that, but perhaps I shouldn't
> ?
>
> I have successfully regstrapped the improvements you suggested.
Thanks. Did you want me to doublecheck the updated patch? Otherwise
feel free to push it to trunk.
> Part 3 of this serie of patches I hope will be regstrapped for
> Friday.
Thanks; I'm impressed at how much progress you've made on this problem.
Dave
