Use std::string_view in eckit_mpi #118
Conversation
wdeconinck
force-pushed
the
feature/eckit_mpi_with_string_view
branch
from
4e10ba5
to
92bc480
Compare
There was a problem hiding this comment.
Aside from my minor comment, I only add that there is the function MPICall (Parallel.cc, and two fwd declarations in ParallelGroup.cc and ParallelRequest.cc) which takes const char* which might by more consistently converted also to std::string_view. I'm only advancing this if it was missed, I have no specific opinion or preference here.
src/eckit/mpi/Comm.cc
Outdated
| @@ -95,10 +95,10 @@ class Environment { | |||
| for (itr = communicators.begin(); itr != communicators.end(); ++itr) { | |||
| eckit::Log::error() << " " << (*itr).first << std::endl; | |||
| } | |||
| throw eckit::SeriousBug(std::string("No communicator called ") + name, Here()); | |||
| throw eckit::SeriousBug(std::string("No communicator called ") + std::string(name), Here()); | |||
There was a problem hiding this comment.
Being pedantic here, for even more terseness one can remove the first std::string construction (in more than one place I see)
There was a problem hiding this comment.
Yeah, that would still work. My nitpick comment is that I prefer braces '{}' for calling the constructor to make it immediately obvious that we are calling the constructor and not a free function. This used to be very helpful for not getting caught out by 'the most vexing parse' behaviour, but these days compilers would warn you about that anyway.
Otherwise, it looks good.
There was a problem hiding this comment.
Thanks! We aim for perfection :) I have implemented all suggestions.
wdeconinck
force-pushed
the
feature/eckit_mpi_with_string_view
branch
from
adb9f96
to
1f5a5af
Compare
This PR changes
const char*arguments intostd::string_view, so that we can now downstream replace amongst otherseckit::mpi::setCommDefault( string.c_str() )andeckit::mpi::comm( string.c_str() )with
eckit::mpi::setCommDefault( string )andeckit::mpi::comm( string ).It should still be compatible with callers passing
const char*.