Introduce p4est_search_partition_gfp

[hannesbrandt]

We add p4est_search_partition_gfp as an alternative to p4est_search_partition_gfx.
The parameter gfq is no longer required as a parameter, which may avoid unnecessary communication in case gfq is not already available locally, but makes the function slightly slower.
To implement p4est_search_partition_gfp with minimal changes, we add p4est_comm_is_empty_gfx, which works like p4est_comm_is_empty_gfq for gfq != NULL and else calls p4est_quadrant_is_equal_piggy on gfp.

[cburstedde]

Thanks! Strictly speaking, if gfq is optional in the comm _gfx function, should it be optional in the partition _gfx function, too? So the assertions only execute if not NULL.

[cburstedde]

Thanks! Strictly speaking, if gfq is optional in the comm _gfx function, should it be optional in the partition _gfx function, too? So the assertions only execute if not NULL.

In that case partition_gfp might directly call partition_gfx (NULL, p).

[hannesbrandt]

I changed the assertions in p4est_search_partition_gfx to check for gfq == NULL || the previous assertions. Now p4est_search_partition_gfp calls p4est_search_partition_gfx with NULL for the gfq parameter.
I added some doxygen commands and made some minor changes in the search.h files like p4est functions being referenced in the p8est file or references not being recognized due to a trailing :.

[cburstedde]

Thanks for the update!

I'm thinking about a more philosophical point: is it a good thing if some arguments of a function may or may not be NULL, legally. This increases coding flexibility but places more burdens on the user to read the API docs in detail. If they do not read them carefully, chances are high that they do something suboptimal. If we were to not allow for NULL defaults wherever sensible, the user might be a happier person and the code might be more stable.

So I know I suggested that the gfq argument is always optional for the gfx functions.
The alternative would be to remove is_empty_gfx, leaving the _gfp and _gfq versions, and to require gfq to always be non-NULL in search_partition_gfx (if I'm not confusing things). This way the user does not need to read the documentation about parameters that may or may not be NULL.

Whout would be your opinion, also checking with @tim-griesbach?

[hannesbrandt]

That is a good point. For a more consistent setup @tim-griesbach and I think we should

• reinstate the qfg != NULL check in search_partition_gfx
• change search_partition_gfp from calling search_partition_gfx to calling search_partition_internal
• add is_empty_gfp to p4est_communication
• remove is_empty_gfx and replace every call of this function in search_partition_internal by something like
  (gfq != NULL) ? is_empty_gfq : is_empty_gfp

What do you think?

[cburstedde]

Thanks for your thoughts. The straightest approach would be to prepare a _gfp and/or _gfq version of a function when there is a mode that works like this, asserting the argument for non-NULL. If there's a mode that benefits from both arguments being available, then add a _gfx version and assert both to be non-NULL. Shall we apply this to both search_partition and is_empty and friends where needed?

Let's not add functions that we do not call though, since dead code is difficult to maintain.

[cburstedde]

That is a good point. For a more consistent setup @tim-griesbach and I think we should

* reinstate the `qfg != NULL` check in `search_partition_gfx`

* change `search_partition_gfp` from calling `search_partition_gfx` to calling `search_partition_internal`

* add `is_empty_gfp` to p4est_communication

* remove `is_empty_gfx` and replace every call of this function in `search_partition_internal `by something like
  `(gfq != NULL) ? is_empty_gfq : is_empty_gfp`

These all sound good. For the last item a macro in the .c may be helpful. These changes seem to be in line with my previous thoughts just posted above.

[cburstedde]

If you might briefly comment on my conversations above I'll be happy to resolve them.

[hannesbrandt]

* reinstate the `qfg != NULL` check in `search_partition_gfx`

* change `search_partition_gfp` from calling `search_partition_gfx` to calling `search_partition_internal`

* add `is_empty_gfp` to p4est_communication

* remove `is_empty_gfx` and replace every call of this function in `search_partition_internal `by something like
  `(gfq != NULL) ? is_empty_gfq : is_empty_gfp`

I've implemented everything according to the list.
Now, the only _gfx function remaining is search_partition_gfx, which actually uses both gfx and gfp. All other related functions are named either _gfq or _gfp and always assume that the respective parameter is not NULL. I've used a macro in search.c to distinguish the cases in search_partition_internal like you proposed.

[cburstedde]

Thanks so much!