> On Sun, 2020-09-20 at 00:32 +0200, Jan Hubicka wrote:
> > Hi,
> > this is cleaned up version of the patch. I removed unfinished bits,
> > fixed
> > propagation, cleaned it up and fixed fallout.
>
> [...]
>
> > While there are several areas for improvements but I think it is not
> > in shape
> > for mainline and rest can be dealt with incrementally.
>
> FWIW I think you typoed:
> "not in shape for mainline"
> when you meant:
> "now in shape for mainline"
> given...
Yep, sorry for that :)
>
> Should new C++ source files have a .cc suffix, rather than .c?
>
> [...]
>
> > + $(srcdir)/ipa-modref.h $(srcdir)/ipa-modref.c \
>
> ...which would affect this ^^^^^^^^^^^^^
I was wondering about that and decided to stay with .c since it is what
other ipa passes use. I can rename the files. We have some sources with
.c extension and others with .cc while they are now all C++. Is there
some plan to clean it up?
> > +void
> > +modref_tree_c_tests ()
> > +{
> > + test_insert_search_collapse ();
> > + test_merge ();
> > +}
> > +
> > +#endif
>
> It looks like these tests aren't being called anywhere; should they be?
>
> The convention is to put such tests inside namespace selftest and to
> call the "run all the tests in this file" function from
> selftest::run_tests.
>
> If you change this to be a .cc file, then the _c_ in the function name
> should become a _cc_.
>
> [...]
>
> > diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> > new file mode 100644
> > index 00000000000..3bdd3058aa1
> > --- /dev/null
> > +++ b/gcc/ipa-modref-tree.h
>
> [...]
>
> > +void modref_c_tests ();
>
> Likewise here; the convention is to declare these within
> selftest.h
>
> [...]
Thanks, indeed is seems that the self tests code was dropped at some
point which I did not noticed. I will fix that up.
>
> Hope this is constructive
It is :)
Honza
> Dave
>
