pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
886 stars 118 forks source link

Problem with usage string_view in API #166

Closed YarikTH closed 2 years ago

YarikTH commented 2 years ago

I caught the following problem:

When I dug a little, I found out that v8pp has a feature detection code, especially for string_view. There shouldn't be any problem with __cpp_lib_void_t and __cpp_lib_logical_traits as soon as the behaviour is identical to the standard ones. There shouldn't be any problem with selecting string_view version in headers if we work with a header-only library. But when string_view type is detected in the header and this type is used in API between the cpp part of the library and end user, then to use a prebuilt version of v8pp user should guess against which c++ standard v8pp is built and they forced to use the same standard as v8pp uses.

Possible solution: make string_view choice at the configuration time and hardcode it into config.hpp. Then at least at the API level both the library and user will use the same type, which completely solves the problem.

pmed commented 2 years ago

Hi Yaroslav,

Thanks for letting me know. Yeah, that seems to be an issue.

The solution you have proposed is great. Let's introduce a #define V8PP_USE_STD_STRING_VIEW in the config.hpp that depends on the std::string_view availability, so this define could be overridden at compile time.

Later a corresponding CMake option would control this behavior.

пн, 9 авг. 2021 г., 18:40 Yaroslav @.***>:

I caught the following problem:

  • Build v8pp with c++14
  • Build my library that using v8pp with c++17
  • Got a lot of sexy "unresolved reference" messages

When I dug a little, I found out that v8pp has a feature detection code, especially for string_view. There shouldn't be any problem with __cpp_lib_void_t and __cpp_lib_logical_traits as soon as the behaviour is identical to the standard ones. There shouldn't be any problem with selecting string_view version in headers if we work with a header-only library. But when string_view type is detected in the header and this type is used in API between the cpp part of the library and end user, then to use a prebuilt version of v8pp user should guess against which c++ standard v8pp is built and they forced to use the same standard as v8pp uses.

Possible solution: make string_view choice at the configuration time and hardcode it into config.hpp. Then at least at the API level both the library and user will use the same type, which completely solves the problem.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pmed/v8pp/issues/166, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAISAUTRBIYH75I4OHUBHXDT37ZIVANCNFSM5B2JA73A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

pmed commented 2 years ago

Added V8PP_USE_STD_STRING_VIEW that is 0 on the master branch with C++14 and is 1 on the c++17 branch.