search

libMesh / TIMPI

Templated Interface to MPI
GNU Lesser General Public License v2.1
12 stars 11 forks source link

Do not use std::remove_if, which moves data #87

Closed loganharbour closed 2 years ago

loganharbour commented 2 years ago

@cticenhour here's the fix. It adds an extra branch within a loop (which is what we tried to avoid 😢)

roystgnr commented 2 years ago

Before we merge I want to turn my commit into a fixup, but more importantly I want to hear back on whether this fixed the problem in Freya.

I usually love the enormity of the "Undefined Behavior can produce nasal demons" joke, but frankly, between "nasal demons" and "your code breaks horribly but only 0.01% of the time", I'd have greatly preferred a trip to the rhinologist.

moosebuild commented 2 years ago

Job Coverage on b92f220 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

cticenhour commented 2 years ago

Hitting this error during libmesh build after the final linking of libdbg, libopt, and libdevel. Do I need to bootstrap or manually configure TIMPI within moose/libmesh/contrib? I was under the impression that configuration was occurring on the libmesh install script level. 

make[4]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib/fparser'
make[3]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib/fparser'
make[2]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib/fparser'
Making all in nanoflann
make[2]: Entering directory '/Users/user/projects/moose/libmesh/build/contrib/nanoflann'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib/nanoflann'
Making all in timpi
make[2]: Entering directory '/Users/user/projects/moose/libmesh/build/contrib/timpi'
make[2]: *** No rule to make target 'all'.  Stop.
make[2]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib/timpi'
make[1]: *** [Makefile:1034: all-recursive] Error 1
make[1]: Leaving directory '/Users/user/projects/moose/libmesh/build/contrib'
make: *** [Makefile:32810: all-recursive] Error 1
roystgnr commented 2 years ago

Do I need to bootstrap

If you switched out the libMesh TIMPI hash (v1.8.1_bootstrapped) for the final hash here? Yeah, exactly. When we actually merge this into libMesh we'll make a 1.8.2 release tag and branch with a bootstrapped tag for the libMesh submodule, but right here there's no bootstrap files in git.

lindsayad commented 2 years ago

I'm going to test this on https://moosedevelopers.slack.com/archives/C010M4X8ZK2/p1642138858010400

cticenhour commented 2 years ago

Getting all green on Freya with this change!

lindsayad commented 2 years ago

This also fixes the failed assertions on the NS input

roystgnr commented 2 years ago

Fantastic. I'm making this 1.8.2 and moving it downstream.

loganharbour commented 2 years ago

Thanks folks. Sorry for the misunderstanding of std::remove_if

roystgnr commented 2 years ago

Hey, don't blame yourself for this. I wrote the too-fragile class that broke, I reviewed the PR that broke it, and before you submitted the PR I specifically answered your question about whether std::remove_if was safe to use here with an overconfident "yes".