bindings: Help C++11 clients avoid calling serial constructor with MPI_Comm

[bradking]

Client code that does not enable ADIOS2_USE_MPI may accidentally try to pass a MPI_Comm instance to our constructor. If the type the MPI library uses to implement MPI_Comm happens to be convertible to one of the types offered by our serial constructors (e.g. bool), the invocation will appear to succeed but will ignore the communicator passed. Add explicitly deleted constructors that have better conversions for common MPI_Comm types.

With this, an attempt in client code to call the adios2::ADIOS constructor with a MPI_Comm object will fail to compile with an error such as:

error: use of deleted function 'adios2::ADIOS::ADIOS(void*)'
    adios2::ADIOS adios(comm);
...note: declared here
    ADIOS(void *define_ADIOS2_USE_MPI_to_use_MPI_constructor) = delete;
    ^~~~~

[bradking]

For reference, this came up in #2420 (comment).

[chuckatkins]

I support this; it's very helpful to have to avoid user errors I think. Can you also add it to the open call in bindings/CXX11/adios2/cxx11/IO.h thugh?

[williamfgc]

@bradking would a explicit constructor be more effective than deleting implicit constructors?

[bradking]

It's not needed for IO::Open because there are no serial overloads that will be selected by passing a MPI_Comm incorrectly. When passed, the communicator is always the third argument, so it should already be a compile error if -DADIOS2_USE_MPI is not enabled.

[bradking]

would a explicit constructor be more effective than deleting implicit constructors?

No. An explicit constructor only prevents implicit calls to that constructor. In this case the call sites are already calling the constructor explicitly. The undesired conversion is taking place in the overload selection of that explicit call.

[williamfgc]

@bradking I see, makes sense. Thanks for the explanation. A minor thing, should this be put in the core:: namespace? That way it propagates to the Python bindings, but I'm not sure if we'd see something similar with Python. Otherwise, it looks good. Thanks! The main issue is that this was failing silently.

[williamfgc]

Just pickin' your brains @bradking and @chuckatkins , is non-target legacy CMake going to have its Python 2 moment?

[bradking]

should this be put in the core:: namespace?

The core::ADIOS constructors are agnostic and take only the helper::Comm abstraction, so this problem cannot occur internally. #2238 already took care of a similar public-facing case for the Python bindings.