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

On OpenBSD string_view is both in boost and std #8955

Merged
merged 3 commits into from
Mar 23, 2020

Conversation

omoerbeek
Copy link
Member

Short description

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

{
bool found = false;
/* early versions of boost::string_ref didn't have the ability to compare to string */
string_view headerNameView(headerName);
boost::string_view headerNameView(headerName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we really would like to use std::string_view when available.. How does lmdb-safe.hh compiles on OpenBSD? Because the string_view detection is a copy-paste from that code..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I normally don't build lmdb support on OpenBSD. But indeed, it gives similar errors.

@rgacogne rgacogne added this to the dnsdist-1.5.0 milestone Mar 20, 2020
@omoerbeek omoerbeek force-pushed the dnsdist-string-view-amb branch 2 times, most recently from 1647ae1 to 133a99c Compare March 20, 2020 13:15
@omoerbeek omoerbeek force-pushed the dnsdist-string-view-amb branch from 133a99c to ae69f67 Compare March 20, 2020 13:19
@RalfvdEnden
Copy link

RalfvdEnden commented Mar 20, 2020

FreeBSD suffers from the same issue. After adding && !defined(__FreeBSD) to the #if line in the patch it builds fine.

@omoerbeek omoerbeek force-pushed the dnsdist-string-view-amb branch from 189757a to 7f073b5 Compare March 20, 2020 16:22
@Habbie
Copy link
Member

Habbie commented Mar 21, 2020

NetBSD 9 does not need this.

@omoerbeek
Copy link
Member Author

lmdb-safe.cc build on OpenBSD now, but warns:

  CXX      ext/lmdb-safe/lmdb-safe.o
ext/lmdb-safe/lmdb-safe.cc:338:10: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
  return std::move(getRWTransaction());
         ^
ext/lmdb-safe/lmdb-safe.cc:338:10: note: remove std::move call here
  return std::move(getRWTransaction());
         ^~~~~~~~~~                  ~
1 warning generated.

@omoerbeek omoerbeek requested a review from rgacogne March 23, 2020 07:15
@rgacogne
Copy link
Member

lmdb-safe.cc build on OpenBSD now, but warns:
[...]

clang++ does warn on Arch as well. AFAICT the std::move() should go as it prevents RVO.

@omoerbeek
Copy link
Member Author

OK, IMO we need a separate PR for that.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the fix for now, but that clearly indicates that we have an issue on some systems where we use C++17's features even though we only require C++11. I hope it will not come back to haunt us later :-/

@rgacogne rgacogne merged commit 6a4590f into PowerDNS:master Mar 23, 2020
@omoerbeek omoerbeek deleted the dnsdist-string-view-amb branch April 20, 2020 08:47
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.

4 participants