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

avoid std::move() on temporary at -werror #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattbenjamin
Copy link

/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/driver/posix/lmdb-safe/lmdb-safe.cc:336:10: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
return std::move(getRWTransaction());
^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/driver/posix/lmdb-safe/lmdb-safe.cc:336:10: note: remove std::move call here
return std::move(getRWTransaction());
^~~~~~~~~~ ~
1 error generated.

/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/driver/posix/lmdb-safe/lmdb-safe.cc:336:10: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
  return std::move(getRWTransaction());
         ^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/driver/posix/lmdb-safe/lmdb-safe.cc:336:10: note: remove std::move call here
  return std::move(getRWTransaction());
         ^~~~~~~~~~                  ~
1 error generated.

Signed-off-by: Matt Benjamin <[email protected]>
@Martchus
Copy link

This PR is likely in vain because this repository is no longer maintained.

I have fixed many more warnings on #15 which you may want to checkout. I have also done lots of further improvements on https://github.com/Martchus/lmdb-safe.

@mattbenjamin
Copy link
Author

I saw that, and I'm going to try to use your repo, if you consider it maintained. If this change is still applicable, would you have accepted it?

@Martchus
Copy link

I would definitely have accepted it. Note l definitely still care about my fork repo as I actively use it in one of my projects. However, I have never spent the time to make it really fancy.

Note that LMDB in general needs a rediciously large address space which is likely a no-go on any 32-bit system. I'm just saying this as a warning so you know what you're getting into.

@mattbenjamin
Copy link
Author

interesting; we're all 64-bit, so that should be ok :)

@mattbenjamin
Copy link
Author

mattbenjamin commented Jun 14, 2023

@Martchus so, is your lmdb-safe no longer standalone--it requires your c++utilities submodule to be present as well?

it seems like that is sort of accidentally true in global.h; would you consider a change to skip that inclusion if the client code defines LMDB_SAFE_STATIC? (since in that case, the code doesn't appear to use anything from c++utilities)

@Martchus
Copy link

It just uses headers and CMake modules from c++utilities but not the library itself. I suppose it would be ok to disable inclusion of c++utilities headers if e.g. LMDB_SAFE_NO_CPP_UTILITIES is defined. That would then disable all features that come with it, e.g. being able to export symbols in case one builds a shared library and the whole lmdb-typed.hh header as it really uses c++utilities headers.

Note that it shouldn't be hard to supply c++utilities as it provides a CMake module and pkg-config module. If you use CMake yourself you can also build it as part of your project (via add_subdirectory).

@mattbenjamin
Copy link
Author

I only need the facilities in lmdb-safe.hh, and it's an advantage to avoid including as little extra as possible, in particular, a whole submodule. It's not about ease of building, it's about maintenance of our larger project.

@Martchus
Copy link

Ok, I would accept commit to switch off c++utilities in lmdb-safe.hh. By the way, have you considered using https://github.com/drycpp/lmdbxx? It seems more popular. I personally only opted for lmdb-safe because of lmdb-typed.hh.

@mattbenjamin
Copy link
Author

well, I had seen it, but liked the interface work in lmdb-safe, liked it's style of c++. what is your opinion of lmdbxx?

@Martchus
Copy link

I haven't used lmdbxx myself yet. I would have chosen it if it would have had an interface similar to lmdb-typed.hh because it seems more popular and is more actively maintained.

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

Successfully merging this pull request may close these issues.

2 participants