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...
>
> Bootstrapped/regtested x86_64-linux including ada, d and go. I plan
> to commit
> it after bit more testing tomorrow.
...this, which suggests the opposite meaning.
I didn't look at the guts of the patch, but I spotted some nits.
> 2020-09-19 David Cepelik <d...@dcepelik.cz>
> Jan Hubicka <hubi...@ucw.cz>
>
> * Makefile.in: Add ipa-modref.c and ipa-modref-tree.c.
> * alias.c: (reference_alias_ptr_type_1): Export.
> * alias.h (reference_alias_ptr_type_1): Declare.
> * common.opt (fipa-modref): New.
> * gengtype.c (open_base_files): Add ipa-modref-tree.h and ipa-
> modref.h
> * ipa-modref-tree.c: New file.
> * ipa-modref-tree.h: New file.
> * ipa-modref.c: New file.
Should new C++ source files have a .cc suffix, rather than .c?
[...]
> + $(srcdir)/ipa-modref.h $(srcdir)/ipa-modref.c \
...which would affect this ^^^^^^^^^^^^^
[...]
> diff --git a/gcc/ipa-modref-tree.c b/gcc/ipa-modref-tree.c
> new file mode 100644
> index 00000000000..e37dee67fa3
> --- /dev/null
> +++ b/gcc/ipa-modref-tree.c
[...]
> +#if CHECKING_P
> +
> +
> +static void
> +test_insert_search_collapse ()
> +{
[...]
> +}
> +
> +static void
> +test_merge ()
> +{
[...]
> +}
> +
> +
> +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
[...]
Hope this is constructive
Dave
