-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix 2 compiler warnings from 1.8.0 #1997
Conversation
- avoid std::move on temporary (fixes clang warning) - avoid creating temporary std::thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing the order of initializers in the HighsOptionsStruct
constructor is obviously fine.
These modifications were also spotted by @HaoZeke, but I'm wary of making the other in case it compromises the behaviour of the task scheduler, since @lgottwald and @galabovaa have only just fixed a serious issue in it.
I need to see a clear explanation as to why the task scheduler is unchanged before this PR will be approved.
Also, we don't accept PRs to master
.
If it were workerThreads.emplace_back(
std::thread(&HighsTaskExecutor::run_worker, i, this)); then it would create a With the With the new code, it will extend the space in the Maybe @lgottwald can just review this part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say makes sense and if @lgottwald is happy I'm happy as well!
The change should not break anything, but is the temporary object not already an r-value reference? What is the compiler warning? |
1 similar comment
The change should not break anything, but is the temporary object not already an r-value reference? What is the compiler warning? |
Apple clang version 15.0.0 (clang-1500.3.9.4) |
Ah ok, that means the compiler suggests to not use move semantics because it the copy would be optimized away which is even better than a move and using a move prevents that.Adding std::move around the expression just makes the move explicit and thereby suppresses the warning.Am 24.10.2024 um 09:10 schrieb Stefan Vigerske ***@***.***>:
In file included from ipx_control.cpp:1:
In file included from src/parallel/HighsParallel.h:16:
In file included from src/parallel/HighsMutex.h:18:
src/parallel/HighsTaskExecutor.h:132:11: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
std::move(std::thread(&HighsTaskExecutor::run_worker, i, this)));
^
src/parallel/HighsTaskExecutor.h:132:11: note: remove std::move call here
std::move(std::thread(&HighsTaskExecutor::run_worker, i, this)));
^~~~~~~~~~ ~
1 warning generated.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Happy to merge this now |
Fixed order of initializers in
HighsOptionsStruct
constructor.Avoid
std::move
on temporarystd::thread
, which gave a clang warning.The idea of
emplace_back
is that one can construct the object that is placed into theworkerThreads
container directly in that container, so one can directly pass the arguments for the constructor of the object to be constructed. (@fwesselm ok?)