Skip to content

fs: work around u8path deprecated-declaration warnings with libc++#25808

Merged
maflcko merged 1 commit intobitcoin:masterfrom
fanquake:fix_24682
Aug 19, 2022
Merged

fs: work around u8path deprecated-declaration warnings with libc++#25808
maflcko merged 1 commit intobitcoin:masterfrom
fanquake:fix_24682

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Aug 9, 2022 •
edited
Loading

When building in c++20 mode using libc++, the following warning is emitted:

./fs.h:72:29: warning: 'u8path<std::string>' is deprecated [-Wdeprecated-declarations]
    return std::filesystem::u8path(utf8_str);
                            ^
/usr/lib/llvm-14/bin/../include/c++/v1/__filesystem/u8path.h:72:27: note: 'u8path<std::string>' has been explicitly marked deprecated here
_LIBCPP_INLINE_VISIBILITY _LIBCPP_DEPRECATED_WITH_CHAR8_T
                          ^
/usr/lib/llvm-14/bin/../include/c++/v1/__config:1042:43: note: expanded from macro '_LIBCPP_DEPRECATED_WITH_CHAR8_T'
                                          ^
/usr/lib/llvm-14/bin/../include/c++/v1/__config:1007:48: note: expanded from macro '_LIBCPP_DEPRECATED'
                                               ^
1 warning generated.

as u8path<std::string> is deprecated starting with C++20.

Fixes: #24682.

@Sjors
Copy link
Member

Sjors commented Aug 10, 2022

tACK 6bf6234790099cce5585b62742ffb572054973e1

Although I don't see any other deprecation warnings, I like that we only suppress this specific one.

@hebasto
Copy link
Member

hebasto commented Aug 10, 2022 •
edited
Loading

I do prefer @MarcoFalke's alternative which also allows to drop the global _SILENCE_CXX20_U8PATH_DEPRECATION_WARNING:

diff --git a/build_msvc/common.init.vcxproj.in b/build_msvc/common.init.vcxproj.in
index f7169a346d..e84ac03623 100644
--- a/build_msvc/common.init.vcxproj.in
+++ b/build_msvc/common.init.vcxproj.in
@@ -90,7 +90,7 @@
       <AdditionalOptions>/utf-8 /Zc:__cplusplus /std:c++20 %(AdditionalOptions)</AdditionalOptions>
       <DisableSpecificWarnings>4018;4244;4267;4334;4715;4805;4834</DisableSpecificWarnings>
       <TreatWarningAsError>true</TreatWarningAsError>
-      <PreprocessorDefinitions>_SILENCE_CXX20_U8PATH_DEPRECATION_WARNING;_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING;ZMQ_STATIC;NOMINMAX;WIN32;HAVE_CONFIG_H;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;_CONSOLE;_WIN32_WINNT=0x0601;_WIN32_IE=0x0501;WIN32_LEAN_AND_MEAN;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING;ZMQ_STATIC;NOMINMAX;WIN32;HAVE_CONFIG_H;_CRT_SECURE_NO_WARNINGS;_SCL_SECURE_NO_WARNINGS;_CONSOLE;_WIN32_WINNT=0x0601;_WIN32_IE=0x0501;WIN32_LEAN_AND_MEAN;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>..\..\src;..\..\src\minisketch\include;..\..\src\univalue\include;..\..\src\secp256k1\include;..\..\src\leveldb\include;..\..\src\leveldb\helpers\memenv;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>
     <Link>
diff --git a/src/fs.h b/src/fs.h
index e8b34319bb..ac58c4a2ba 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -69,7 +69,11 @@ public:
 
 static inline path u8path(const std::string& utf8_str)
 {
+#if __cplusplus < 202002L
     return std::filesystem::u8path(utf8_str);
+#else
+    return std::filesystem::path(std::u8string{utf8_str.begin(), utf8_str.end()});
+#endif
 }
 
 // Disallow implicit std::string conversion for absolute to avoid

@fanquake
Copy link
Member Author

fanquake commented Aug 10, 2022

I do prefer @MarcoFalke's #24682 (comment)

I don't really want to start introducing __cplusplus version checks into our code.

which also allows to drop the global _SILENCE_CXX20_U8PATH_DEPRECATION_WARNING:

Why does dropping that matter? It's being used as intended.

@maflcko
Copy link
Member

maflcko commented Aug 10, 2022

No opinion on what to do, but the new code should ideally be the version we'll adopt once (and if) we bump to C++20

ryanofsky
ryanofsky approved these changes Aug 15, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 6bf6234790099cce5585b62742ffb572054973e1. I do think hebasto/marco's solution #25808 (comment) is cleaner, more standard, and more future proof but both solutions are good.

@fanquake @hebasto
fs: work around u8path deprecated-declaration warnings with libc++
ced00f5 
When building in c++20 mode using libc++, the following warning is
emitted:
```bash
./fs.h:72:29: warning: 'u8path<std::string>' is deprecated [-Wdeprecated-declarations]
    return std::filesystem::u8path(utf8_str);
                            ^
/usr/lib/llvm-14/bin/../include/c++/v1/__filesystem/u8path.h:72:27: note: 'u8path<std::string>' has been explicitly marked deprecated here
_LIBCPP_INLINE_VISIBILITY _LIBCPP_DEPRECATED_WITH_CHAR8_T
                          ^
/usr/lib/llvm-14/bin/../include/c++/v1/__config:1042:43: note: expanded from macro '_LIBCPP_DEPRECATED_WITH_CHAR8_T'
                                          ^
/usr/lib/llvm-14/bin/../include/c++/v1/__config:1007:48: note: expanded from macro '_LIBCPP_DEPRECATED'
                                               ^
1 warning generated.
```

as u8path<std::string> is deprecated starting with c++20.

Fixes: bitcoin#24682.

Co-authored-by: MacroFake <[email protected]>
Co-authored-by: Hennadii Stepanov <[email protected]>
@fanquake
Copy link
Member Author

fanquake commented Aug 19, 2022

I'm still not a fan of the version checking, but I've pushed up the alternative solution.

@fanquake fanquake changed the title fs: suppress u8path deprecated-declaration warnings with libc++ fs: work around u8path deprecated-declaration warnings with libc++ Aug 19, 2022
maflcko
maflcko reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK ced00f5

Looks like this creates 3 copies of the string, but they should all be moved so it has probably no performance impact. Regardless, the helper shouldn't be used in hot loops anyway.

hebasto
hebasto approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK ced00f5

@maflcko maflcko merged commit d480586 into bitcoin:master Aug 19, 2022
@fanquake fanquake deleted the fix_24682 branch August 19, 2022 13:46
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 19, 2022
Merge bitcoin#25808: fs: work around u8path deprecated-declaration wa…
241e31f 
…rnings with libc++

ced00f5 fs: work around u8path deprecated-declaration warnings with libc++ (fanquake)

Pull request description:

  When building in c++20 mode using libc++, the following warning is emitted:
  ```bash
  ./fs.h:72:29: warning: 'u8path<std::string>' is deprecated [-Wdeprecated-declarations]
      return std::filesystem::u8path(utf8_str);
                              ^
  /usr/lib/llvm-14/bin/../include/c++/v1/__filesystem/u8path.h:72:27: note: 'u8path<std::string>' has been explicitly marked deprecated here
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_DEPRECATED_WITH_CHAR8_T
                            ^
  /usr/lib/llvm-14/bin/../include/c++/v1/__config:1042:43: note: expanded from macro '_LIBCPP_DEPRECATED_WITH_CHAR8_T'
                                            ^
  /usr/lib/llvm-14/bin/../include/c++/v1/__config:1007:48: note: expanded from macro '_LIBCPP_DEPRECATED'
                                                 ^
  1 warning generated.
  ```

  as [`u8path<std::string>`](https://en.cppreference.com/w/cpp/filesystem/path/u8path) is deprecated starting with C++20.

  Fixes: bitcoin#24682.

ACKs for top commit:
  MarcoFalke:
    review ACK ced00f5
  hebasto:
    ACK ced00f5

Tree-SHA512: f012c4f0bec691090eb3ff128ee0cdc392f73e7857b97131da924ab18c088a82d2fba95316d405feb8b744cba63bfeff7b08143086c173fddbf972139ea0ac0b
@bitcoin bitcoin locked and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

u8path<std::string> deprecation with libc++ and c++20

5 participants

@fanquake @Sjors @hebasto @maflcko @ryanofsky