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

Suspicious changes to is_cstr / is_cstr_or_string #1330

Closed
cschreib opened this issue Mar 26, 2022 · 1 comment · Fixed by #1331
Closed

Suspicious changes to is_cstr / is_cstr_or_string #1330

cschreib opened this issue Mar 26, 2022 · 1 comment · Fixed by #1331
Assignees
Labels
Bug.Aaah! Pure horror. Bug.Derp They don't call me The Phantom Derpstorm for no reason.
Milestone

Comments

@cschreib
Copy link
Contributor

cschreib commented Mar 26, 2022

I have been using the v4.0.0-alpha branch for a while, and thought I would upgrade to the tip of the develop branch to get the latest fixes and improvements. Unfortunately there seems to be a regression, because my project no longer runs correctly. I have struggled to debug what the issue exactly is, and to reproduce a minimal example, so instead I just investigated with a git bisect in sol2.

I narrowed it down to ff783a1, which changes meta::is_cstr_of to not include std::string.

I noticed a few things that looked suspicious to me, and after fixing them, my issues disappear. Some of it might be my misunderstanding of what the code does, sol2 is very complex, but it seems to have done the trick.

  1. Although most occurences of is_cstr_v in "stack_field.hpp" were changed to is_cstr_or_string_v, there was one line that used to be

    if constexpr (meta::is_c_str_v<T> || meta::is_string_of_v<T, char>) {

    and got changed to just
    if constexpr (meta::is_c_str_v<T>) {

    That seems like an omission to me, so I changed it to is_cstr_or_string_v<T>.

  2. In "usertype_storage.hpp", the code now reads

    template <typename K, typename Fq, typename T = void>
    struct binding : binding_base {
    using uF = meta::unqualified_t<Fq>;
    using F = meta::conditional_t<meta::is_c_str_or_string_of_v<uF, char>
    #if SOL_IS_ON(SOL_CHAR8_T_I_)
    || meta::is_c_str_or_string_of_v<uF, char8_t>
    #endif
    || meta::is_c_str_or_string_of_v<uF, char16_t> || meta::is_c_str_or_string_of_v<uF, char32_t> || meta::is_c_str_or_string_of_v<uF, wchar_t>,
    std::add_pointer_t<std::add_const_t<std::remove_all_extents_t<Fq>>>, std::decay_t<Fq>>;
    F data_;

    I assume this code is meant to transform stuff like Fq=char[26] into const char* for storage. But this looks weird, as it would turn Fq=std::string into F=const std::string*, which would lead to a dangling pointer once the std::string goes out of scope. I changed it to use is_c_str_of only.

  3. In "traits.hpp"

    sol2/include/sol/traits.hpp

    Lines 701 to 702 in 50b62c9

    template <typename T>
    constexpr inline bool is_c_str_or_string_v = is_c_str<T>::value;

    Looks wrong. I changed it to = is_c_str_or_string<T>::value.

It was after fixing point 3 that my problem disappeared.

@ThePhD
Copy link
Owner

ThePhD commented Apr 14, 2022

Hey, thanks for reviewing this. The (1) one was intentional on my part. The rest I'll merge!

@ThePhD ThePhD self-assigned this Apr 14, 2022
@ThePhD ThePhD added Bug.Aaah! Pure horror. Bug.Derp They don't call me The Phantom Derpstorm for no reason. labels Apr 14, 2022
@ThePhD ThePhD added this to the Bugs milestone Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug.Aaah! Pure horror. Bug.Derp They don't call me The Phantom Derpstorm for no reason.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants