Add string_view API for c++17 #410

roligugus · 2020-11-03T19:40:58Z

Follow-up of WIP: #80 to add std::string_view support.

std::string_view is enabled if cxx is compiled with c++17 or later. One will need to set the c++17 (or later) feature on the cxx crate for cargo-based builds. Also added a "CXX Features" section to the cargo build info if that helps.

Notes:

This only replaces the std::string ctor, not the ptr and ptr/len ctors as in the WIP
- We could replace the latter ctors as well, but it can break client code. It would require changes for rust::str and rust::String parameters and return values in the client code. Not sure you'd want to break the API.
Tests are only enabled for c++17 (or later)
- One needs to set the c++17 (or later) feature on cxx in tests/ffi/Cargo.toml at this time. Not ideal, but a start.
Added a "CXX Features" section to the documentation if that helps

README.md

roligugus · 2020-11-03T19:44:34Z

include/cxx.h

+#error "string_view gets auto-detected"
+#endif
+
+// clang-format off


Disabling clang-format as settings will flatten everything which makes this hard to follow.

Using normal feature detection for compilers supporting it. Feature detection was also noted in the WIP PR.
Tested paths on godbolt.org as I don't have access to all the compiler versions.

Idea is that if string_view is supported, we should support it.

roligugus · 2020-11-03T19:46:23Z

src/cxx.cc

@@ -65,7 +65,11 @@ static void initString(String *self, const char *s, size_t len) {
  }
 }

+#ifdef CXXBRIDGE_HAS_STRING_VIEW
+String::String(std::string_view sv) : String(sv.data(), sv.length()) {}


Forwarding it to ptr/len ctor that does the nice asserts.

FYI, empty string_view has nullptr and 0 size.

roligugus · 2020-11-03T19:47:42Z

include/cxx.h

@@ -14,6 +14,45 @@
 #include <basetsd.h>
 #endif

+#ifdef CXXBRIDGE_HAS_STRING_VIEW


Using CXXBRIDGE_.. naming to align more with other defines. WIP used CXX_... naming

Bikeshedding time?

roligugus · 2020-11-03T19:48:47Z

tests/ffi/tests.cc

@@ -598,9 +598,26 @@ extern "C" const char *cxx_run_test() noexcept {
  r_take_unique_ptr(std::unique_ptr<C>(new C{2020}));
  r_take_ref_c(C{2020});
  r_take_str(rust::Str("2020"));
+  r_take_empty_str(rust::Str(""));


Added "empty" tests for std::string_view and figured we might as well add tests for rust::str and rust::String

roligugus · 2020-11-03T20:18:44Z

include/cxx.h

+#else
+  // no __has_include
+  #if _MSC_VER >= 1910L && _MSVC_LANG > 201402L
+    // msvc: 19.10 requires c++latest (!)


Older msvc compilers that supported string_view. Not sure what compiler versions you want to support, but I've included the ones I could get my hands on through godbolt.

There could be a discussion if it would make sense to support "c++latest" in cxx as a feature (for msvc).

dtolnay

Thanks for the PR! This is great. I am going to defer reviewing this for a little bit while I get the https://cxx.rs site wrapped up and published. That will also provide a better place to document the features situation -- I think it's not good for a readme to go into this level of detail.

roligugus · 2020-11-24T20:40:03Z

That will also provide a better place to document the features situation -- I think it's not good for a readme to go into this level of detail.

No pressure to look at this. Just had some downtime at work to quickly run after this.

Rebased changes and I've moved the "features" documentation into the book/build/cargo.md. Not sure if you want to have it there or somewhere else. Just a placeholder, really.

roligugus · 2020-11-24T20:41:23Z

book/src/build/cargo.md

@@ -144,6 +144,46 @@ manifest key][links]* in the Cargo reference.

 [links]: https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key

+## Features


Not sure if that's the place where you'd want to document features if at all. Just a placeholder.

Nicer to look at page: https://github.com/roligugus/cxx/blob/string_view/book/src/build/cargo.md#features

dtolnay · 2020-11-25T00:17:58Z

tests/ffi/tests.cc

+  r_take_rust_string(std::string_view("2020"));
+  r_take_empty_rust_string(std::string_view{});
+  r_take_empty_rust_string(std::string_view(""));
+  const auto rustString = rust::String("2020"); // avoid lifetime issue


What lifetime issue is this comment referring to?

"Lifetime issue" was referring to lifetime of (temporary) rust::String("2020") needing to exceed lifetime of string_view in ASSERT() below.

Was easier to use defensive programming than trying to figure it out exactly. Anyhow, checked and the temporary rust::String("2020") is only destructed after the comparison.

Will remove this var and merge code into ASSERT().

dtolnay

I am concerned that in this implementation, simply adding a new dependency to a Cargo build graph can cause existing working code to fail to link. If the newly added crates somewhere contain a dependency on "cxx/c++17", that leads to removing various symbols from the cxx rlib without doing anything to eliminate use of those symbols in elsewhere in the build graph.

Relatedly it seems like this implementation violates ODR which is UB (https://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule), because the class definitions of Str and String don't necessarily "consists of the same sequence of tokens" each place they appear.

Thoughts? It seems like more design work is still required on how to integrate this from the build side.

roligugus · 2020-11-26T23:52:19Z

I am concerned that in this implementation, simply adding a new dependency to a Cargo build graph can cause existing working code to fail to link. If the newly added crates somewhere contain a dependency on "cxx/c++17", that leads to removing various symbols from the cxx rlib without doing anything to eliminate use of those symbols in elsewhere in the build graph.

Yes, that is a downside of tying that functionality to a C++ standard switch and automatically enabling it in the header if it's available while at the same time removing existing functionality. The nice thing is that you get link errors due to missing symbols that your particular client compilation requires.

UPDATE: Removed paragraph as I think I understood wrongly what you meant. The other stuff here still applies, imho.

The current approach would not work for these cases as it breaks the cxx lib ABI as well as the API.

In order to always be able to handle this case, one must never remove current functionality to be backwards compatible from an ABI point-of-view. One could be somewhat flexible with APIs depending on the change - more on that later. Yeah, there's also the "let's version APIs and lib" approach that one would do in C++/C land (not sure about rust), but I don't think that worth considering as that's always a pain and I typically like avoiding that like the pest.

Possible approach to follow...

roligugus · 2020-11-26T23:53:35Z

Relatedly it seems like this implementation violates ODR which is UB (https://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule), because the class definitions of Str and String don't necessarily "consists of the same sequence of tokens" each place they appear.

Don't think this is an ODR problem in this instance. The cxx rlib itself is fine if compiled e.g. with C++17. No ODR there. Your client code is also fine even though your included cxx header might declare different things. No ODR there. However, you will run into link errors when trying to link the cxx rlib into your client lib/exec conveniently avoiding ODR.

It only would become an ODR issue if you were compiling cxx and your client TUs directly into the same executable or lib without going through a separate cxx lib. However, in that case you will be using your own build system and you better be compiling everything linked together into the same lib/exec using the same compilation settings.

roligugus · 2020-11-27T00:22:27Z

Thoughts? It seems like more design work is still required on how to integrate this from the build side.

Agree. So what can we do to avoid that?

The problem is that this PR breaks the API and the ABI by adding a new string_view ctor API (with implementation) and removing another string ctor API (with implementation). In addition it does this silently depending on how the client using cxx is compiled. Such an API and ABI change is only workable if you control all code and are willing to set the same compilation options used to toggle APIs in cxx.

A way of handling that is not to automatically enable the string_view APIs
Using a #define to enable the string_view API on demand while using it in your client code has the same issue as automatically enabling it upon a certain c++ standard. Different clients might have different defines resulting in the same issues we try to avoid.

The only way to handle that would be to do something similar to what e.g. autotools do. The idea is to bake the functionality into cxx. E.g. have the cxx crate write a config.h equivalent that #defines what features are enabled during the cxx lib build and then have cxx.h include that generated header. This way, the functionality that cxx supports (defined by its features) is defined by cxx itself and not by the dependencies using it.

However, you still would run into the same issue for breaking changes where client A needs cxx features X and client B needs cxx feature Y. So that would not be solved as the cxx lib compilation would define the supported functionality depending on the features defined.

A better way to fix that for this change is to "only add new APIs".
Essentially chosing the approach of not breaking the cxx rlib ABI. APIs still can be changed in ways that don't break the existing ABI. In this case, we can add the string_view APIs, while keeping the string APIs around.

Since we're currently keeping the char*-parameter constructors, we should be able to simply add the string_view-parameter constructor a la:

#ifdef CXXBRIDGE_HAS_STRING_VIEW
  String(std::string_view);
#endif
  String(const std::string &);
  String(const char *);
  String(const char *, size_t);

Ditto with the conversion operator.

This will keep the ABI backwards-compatible, i.e. client code compiled with e.g. c++14 will be able to compile and link against a cxx rlib compiled with c++17. This allows to compile the cxx lib with whatever highest c++ standard required to support both c++14 (using string ctor) clients and c++17 clients (using string_view ctor).

We can go further than that. By inlining the new string_view APIs in the cxx.h header, we can even support the "other direction", i.e. C++17 clients using the string_view APIs compiled against a cxx lib compiled with c++14. The trick is in the inlined "C++17 string_view API" code that essentially gets injected into the client compilation via cxx.h due to inlining and internally only calling existing C++14/11 cxx APIs.

We can keep the automatic enabling of the string_view ctor and conversion operators to support string_view APIs.

Sounds like a better approach? I can update this PR to give a better picture.

roligugus · 2020-11-27T01:51:50Z

book/src/build/cargo.md

+<tr><td>c++17</td><td>-std=c++17 (gcc, clang), /std:c++17 (msvc)</td><td>Compile CXX lib with c++17</td></tr>
+</table>
+
+## Additional C++ standard features


Potential documentation for enabling additional C++ APIs.

Nicer to look at page:
https://github.com/roligugus/cxx/blob/string_view/book/src/build/cargo.md#additional-c-standard-features

roligugus · 2020-11-27T02:09:29Z

include/cxx.h

@@ -34,6 +73,9 @@ class String final {
  String(String &&) noexcept;
  ~String() noexcept;

+#ifdef CXXBRIDGE_HAS_STRING_VIEW
+  String(std::string_view sv) : String(sv.data(), sv.length()) {}


We should add "always inline", e.g. __attribute__((always_inline)) (gcc/clang) and __forceinline (msvc), to make sure it gets inlined even if e.g. people disable inlining, etc.

hallfox · 2023-07-08T02:59:00Z

Hi there! I went looking for this feature earlier today, seems like it's stalled out. Is there any interest in revisiting this?

roligugus commented Nov 3, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

roligugus commented Nov 3, 2020

View reviewed changes

dtolnay reviewed Nov 10, 2020

View reviewed changes

roligugus force-pushed the string_view branch 2 times, most recently from d3eb663 to 18d9c71 Compare November 24, 2020 20:36

roligugus commented Nov 24, 2020

View reviewed changes

dtolnay reviewed Nov 25, 2020

View reviewed changes

dtolnay requested changes Nov 25, 2020

View reviewed changes

roligugus added 2 commits November 26, 2020 20:18

Add string_view API for c++17

6c3251f

Add features documentation

e135131

roligugus force-pushed the string_view branch from 18d9c71 to e135131 Compare November 27, 2020 01:50

roligugus commented Nov 27, 2020

View reviewed changes

adetaylor mentioned this pull request Mar 8, 2021

Support for std::string_view (C++17) #734

Open

anforowicz mentioned this pull request Mar 24, 2025

Add an explicit conversion operator from rust::Str to std::string_view. #1486

Merged

		@@ -144,6 +144,46 @@ manifest key][links]* in the Cargo reference.

		[links]: https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key

		## Features

Add string_view API for c++17 #410

Are you sure you want to change the base?

Add string_view API for c++17 #410

Uh oh!

Conversation

roligugus commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roligugus Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

roligugus commented Nov 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

roligugus commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roligugus commented Nov 26, 2020

Uh oh!

roligugus commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallfox commented Jul 8, 2023

Uh oh!

Uh oh!

roligugus commented Nov 3, 2020 •

edited

Loading

roligugus Nov 3, 2020 •

edited

Loading

roligugus commented Nov 26, 2020 •

edited

Loading

roligugus commented Nov 27, 2020 •

edited

Loading