Skip to content
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

Revert a change that breaks compilation. #15458

Merged
merged 1 commit into from Jun 23, 2023
Merged

Conversation

bangerth
Copy link
Member

This reverts a part of #15422 (namely, https://github.com/dealii/dealii/pull/15422/files#diff-2c3b4f7640b5ae1a3b05deecf96447b2e09a22e8368ac24271b692d7eb6b29d1) and is necessary for me to build the library on my laptop.

@masterleinad Do your recall why you needed to make that change?

@bangerth bangerth added this to the Release 9.5 milestone Jun 23, 2023
@tamiko
Copy link
Member

tamiko commented Jun 23, 2023

In reference to #15383, and slightly faster than #15459

@tamiko
Copy link
Member

tamiko commented Jun 23, 2023

I think clang-16 and gcc-13 have a different understanding of this corner case and a std::move is no longer necessary. Current master compiles for me with gcc-13 and clang-16 but older versions do fail.

@bangerth
Copy link
Member Author

"Slightly faster"? I beat you by a full 20 minutes! ;-)

@tamiko tamiko merged commit 5a2aeb5 into dealii:master Jun 23, 2023
14 checks passed
@bangerth bangerth deleted the symengine branch June 23, 2023 20:59
@masterleinad
Copy link
Member

@masterleinad Do your recall why you needed to make that change?

clang-tidy was reporting that we return by const reference and that std::move would not have any effect.

@bangerth
Copy link
Member Author

I see. I have to admit that I don't understand the warning then. The code in question is

      SE::PiecewiseVec piecewise_function;
      [...]
      expression = SE::piecewise(std::move(piecewise_function));

and we're calling the following function in SymEngine:

inline RCP<const Basic> piecewise(PiecewiseVec &&vec)
{
    return make_rcp<Piecewise>(std::move(vec));
}

Since piecewise() takes an rvalue reference, the std::move does make sense.

But perhaps I'm missing something. If you happen to have the warning message somewhere (or get it again, now that we have the original code back), feel free to open an issue!

@bangerth
Copy link
Member Author

(It is of course also possible that we're using different versions of SymEngine and that that causes the difference. For reference, the function piecewise() in question is in symengine-0.8.1/include/symengine/logic.h, line 101, in my installation.)

@tamiko
Copy link
Member

tamiko commented Jun 25, 2023

@bangerth The clang-16 and gcc-13 variants that accepted the removal of the std::move have symengine 0.10 installed, the once that failed (and hence the revert) are sitting on symengine 0.90 or older.

@bangerth
Copy link
Member Author

So the newer variants are ok with both forms? In that case, we should leave things as they currently (after reverting) are, right?

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

Successfully merging this pull request may close these issues.

None yet

4 participants