Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Optionally disable default CommWorld args #85

Merged
merged 8 commits into from Apr 10, 2013

Conversation

Projects
None yet
4 participants
Owner

roystgnr commented Apr 9, 2013

The library doesn't yet compile with all new default Communicator
arguments disabled, because some of those arguments follow other
arguments with defaults. We'll need to add some overload shims to fix
that.

Owner

roystgnr commented Apr 9, 2013

Let's not actually pull this until the overload shims are ready, but it's at a stage worth perusing now, if only to see if anyone dislikes my choice of macro name.

Owner

roystgnr commented Apr 9, 2013

Hmm... the trick with shims is that, if we really want to catch every --disable-default-comm-world error at compile time, we're going to need to ifdef out some of the old APIs entirely. There's no way for a "Mesh mesh(3);" construction to be caught without actually disabling the API whose first argument is an integer-with-a-default, for example, because you can't follow an integer-with-a-default argument with an anything-without-a-default argument.

So we might as well leave the Mesh(int dim=1) constructor as it was, in that case, and then add our new canonical constructor as a separate overload, which would either be (A) "Mesh(int dim, Communicator comm)" or (B) "Mesh(Communicator comm, int dim=1)". Option (C) would be to just never disable the default comm at compile time for that method and others, so we'd keep the new API as-is but we'd only catch errors in those methods at runtime.

I'd choose option (B), but I'll implement (A) or (C) instead if others prefer.

Owner

benkirk commented Apr 9, 2013

I'm fine with the macro name.

I prefer (B), but would be OK implementing (B) only without the Mesh(int dim=1) constructor fallback. Since we can't use delegating constructors without C++ 2011 multiple, redundant constructors for these classes is a pain. (B) still provides the default constructor we've been pushing as best practices now for a while.

For things like

  PetscVector (
const numeric_index_type n,
const numeric_index_type n_local,
const ParallelType type = AUTOMATIC,
const Parallel::Communicator &comm = libMesh::CommWorld);

which are hid behind a build() member, I agree with Derek that I'd just as soon reorder the constructor to

  PetscVector (
const Parallel::Communicator &comm,
const numeric_index_type n, 
const numeric_index_type n_local,
const ParallelType type = AUTOMATIC);

than maintain redundant constructor for advanced, non-demonstrated usage.

Owner

roystgnr commented Apr 9, 2013

Yeah, I'm now convinced on the "let's break the constructors hidden behind build() methods" issue too.

I'll get started on (B), I'll implement any necessary fallback constructors where they're easy to do, and I'll make up a list of the stuff like PetscVector where fallbacks would be a pain in the neck.

Owner

friedmud commented Apr 9, 2013

I concur. ;-)

Just to let you know that I am paying attention, but have nothing to add ;-)

roystgnr added some commits Apr 9, 2013

Optionally disable default CommWorld args
The library doesn't yet compile with all new default Communicator
arguments disabled, because some of those arguments follow other
arguments with defaults.  We'll need to add some overload shims to fix
that.
Add LIBMESH_CAN_DEFAULT_TO_COMMWORLD macro
And reorder new method args as necessary to support it.

Because I just redid our examples to use the new methods on Mesh
classes and because that was one of the necessary reorderings, this
won't pass "make check" yet; a subsequent commit should fix that.

roystgnr added some commits Apr 10, 2013

Reorder one more communicator argument
Not sure how I missed this before, but we need another reordering to
make things workable with disabled default comm arguments.
Fix bugs caught by disable-default-comm-world
This new feature is already paying for itself!
Owner

roystgnr commented Apr 10, 2013

I did a pull --rebase and push --force to bring in the other branch's examples changes after they went into master; apologies if anyone was pulling from this branch.

We appear to be passing "make check" with both the default config and with --disable-default-comm-world as of d5d29fb. Ready to pull whenever.

Owner

benkirk commented Apr 10, 2013

Awesome, thanks for doing this. I'm OK with merging this now.

Just to force the issue, how would you feel about #ifdef - disabling e.g. the Parallel::send() wrappers too when configuring with this option?

I'll be happy to do that after you merge this, though...

Owner

roystgnr commented Apr 10, 2013

Gah - of course we need to ifdef out the global Parallel:: functions too!

Sorry I forgot; thanks for volunteering.

roystgnr added a commit that referenced this pull request Apr 10, 2013

Merge pull request #85 from roystgnr/disable_default_args
Optionally disable default CommWorld args

@roystgnr roystgnr merged commit c112ea2 into libMesh:master Apr 10, 2013

@roystgnr roystgnr deleted the roystgnr:disable_default_args branch Apr 10, 2013

@roystgnr roystgnr referenced this pull request in libantioch/antioch Jul 20, 2013

Closed

A couple of improvements for VexCL utilities #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment