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.
Though perhaps *not* duplicating the tests but rather simply specify
-fno-exceptions,
with a comment "Fails with exceptions" may be better.
> * c-c++-common/analyzer/file-pr58237-noexcept.c: New test,
> based on gcc.dg/analyzer/file-pr58237.c.
>
> > * c-c++-common/analyzer/fopen-2.c: New test.
>
> Looks fopen-2.c is a move of the parts of gcc.dg/analyzer/fopen-1.c
> that can also be C++, so please state that in the ChangeLog.
>
Nods.
[...snip...]
> Nice - thanks.
>
> Can this be combined with the EQ_EXPR and NE_EXPR cases? (possibly
> updating the comment) The code looks identical to me, but I might be
> misreading it.
>
>
Totally, I forgot to move it after making sure it worked.
Thanks.
> [...snip...]
>
> > diff --git a/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c
> b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c
> > new file mode 100644
> > index 00000000000..b208f58f09f
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c
> > @@ -0,0 +1,72 @@
> > +#include <stdlib.h>
> > +
> > +struct ptr_wrapper
> > +{
> > + int *ptr;
> > +};
> > +
> > +struct ptr_wrapper
> > +test_1 (void)
> > +{
> > + struct ptr_wrapper r;
> > + r.ptr = (int *) malloc (sizeof (int));
> > + return r;
> > +}
>
> This looks the same as gcc.dg/analyzer/compound-assignment-1.c
>
> Should this be a move, rather than a new file? i.e. is the patch
> missing a deletion of the file in the old location?
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c
> b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c
> > new file mode 100644
> > index 00000000000..6b7d25cfabe
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c
>
> Likewise here for infinite-recursion.c.
>
> [...snip...]
>
> > diff --git
> a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> > similarity index 97%
> > rename from
> gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> > rename to
> gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> > index 0172c9b324c..1b657697ef4 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> > +++
> b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c
> > @@ -1,6 +1,6 @@
> > #include <stdlib.h>
> >
> > -#include "analyzer-decls.h"
> > +#include "../../gcc.dg/analyzer/analyzer-decls.h"
> >
> > struct iter
> > {
> > @@ -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.
> > /* 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.
Part 3 of this serie of patches I hope will be regstrapped for Friday.
Thanks,
Benjamin.
