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

misc/cpp/imgui_stdlib.h (was: misc/stl/imgui_stl.h) #2035

Closed
ocornut opened this issue Aug 22, 2018 · 16 comments
Closed

misc/cpp/imgui_stdlib.h (was: misc/stl/imgui_stl.h) #2035

ocornut opened this issue Aug 22, 2018 · 16 comments

Comments

@ocornut
Copy link
Owner

ocornut commented Aug 22, 2018

EDIT: Renamed to misc/cpp/imgui_stdlib.h

Just a little news post to state that I have added this file misc/stl/imgui_stl.h in the repo, which is an example of how to wire a std::string-like type to InputText() with the new ImGuiInputTextFlags_CallbackResize option.

The two functions exposed in imgui_stl.h are currently:

bool  InputText(const char* label, std::string* str, ImGuiInputTextFlags flags = 0, ImGuiInputTextCallback callback = NULL, void* user_data = NULL);
bool  InputTextMultiline(const char* label, std::string* str, const ImVec2& size = ImVec2(0, 0), ImGuiInputTextFlags flags = 0, ImGuiInputTextCallback callback = NULL, void* user_data = NULL);

Also see #2006 for details, but basically a simple wrapper for std:string would look like:

static int InputTextCallback(ImGuiInputTextCallbackData* data)
{
    if (data->EventFlag == ImGuiInputTextFlags_CallbackResize)
    {
        // Resize string callback
        std::string* str = (std::string*)data->UserData;
        IM_ASSERT(data->Buf == str->c_str());
        str->resize(data->BufTextLen);
        data->Buf = (char*)str->c_str();
    }
}

bool ImGui::InputText(const char* label, std::string* str, ImGuiInputTextFlags flags)
{
    flags |= ImGuiInputTextFlags_CallbackResize;
    return InputText(label, (char*)str->c_str(), str->capacity() + 1, flags, InputTextCallback, (void*)str);
}

Feedback greatly welcome. Let me know if either the imgui_stl helper of the new ImGuiInputTextFlags_CallbackResize flag works for you.

@wc-duck
Copy link

wc-duck commented Aug 27, 2018

I'm not entirely sure but I think I have been bitten by this before. Isn't casting away const as you do with (char*)data.c_str() undefined behavior?
I would go with &data[0] instead, but I might be wrong, c++ works in mysterious ways :(

@ocornut
Copy link
Owner Author

ocornut commented Aug 27, 2018

I think this should work with any modern implementation that's not ref-counted (with shared non-empty data). A quick google essentially leads to "C++11 added specific language forbidding std::string from being reference counted".

I'd be happy to hear about any concrete problem if any - there probably will be some - but I'd rather avoid getting too deep in the game of C++-theoretical-what-ifs without concrete evidence.

As I understand, .data[0] provide even less guarantees, aka no guarantee to have a null-terminator (though in practice it will be the same).

@wc-duck
Copy link

wc-duck commented Aug 27, 2018

I just brought it up as a heads up as I remember doing something like it and having msvc doing something "smart" that GCC and clang didn't do :( unfortunately I don't remember more clearly than that and that is not much help I know so feel free to ignore.

@ocornut
Copy link
Owner Author

ocornut commented Aug 27, 2018

Will keep in mind, thanks!
It seems to work here with the STL of VS2010, VS2015 and VS2017.

@jdumas
Copy link
Contributor

jdumas commented Aug 28, 2018

Honestly writing to the internal buffer of a std::string pre-C++11 is just asking for trouble. There is nothing in the standard that guarantees the raw buffer to be null-terminated. In practice gcc and clang seem to do the reasonable thing, while older versions of MSVC might not work. Maybe they do, but maybe you're doing an out-of-bound access and the last byte is null and you're lucky. But this will always be a gamble in C++98.

This is not a problem anymore with C++11 since the standard guarantees the internal buffer of a string is null-terminated. This means using s.data(), s.c_str() or &s[0] in C++11 will yield the same result. I prefer to use s.data() this it better conveys the meaning of the operation, but this function already exists in C++98 for std::string and might return a pointer to a non null-terminated buffer (unlikely in practice though). I also remember reading somewhere that &s[0] should work even if the string is empty, whereas &s.front() does not (or maybe it's only for std::vector). Can't find anything about that on cppreference though.

Anyway, in the end, you can always use macros to guard against users including this code pre-C++11 if you want to be on the safe side of things...

@eliasdaler
Copy link
Contributor

Thanks for this, maybe with this I'll be able to remove all my extensions to ImGui. :)

@jdumas agreed.

@ocornut, There's no full-proof way to modify internal buffer pre-C++11, sadly. There's no guarantee that the buffer will be null-terminated and it's not even guaranteed to be contiguous! So I think we need to warn users about C++11 requirement...

As for the &s[0], read this - looks dangerous.

So, to be perfectly safe, we either need to make an assert which will check that the passed string is not empty, or do "reserve" for at least 1 char to make &s[0] work well for all the compilers. I'll research a bit more on this topic later.


By the way, I think it'll be better to not use "STL", but some for of "C++ std" in namings. Please, read this on topic of "STL" vs "std", but the main idea is: "STL" =/= "standard library" and most people in C++ world move away from referring to the Standard Library as "STL".

@ocornut
Copy link
Owner Author

ocornut commented Aug 31, 2018

As soon as we dip an inch into C++ the bike-shedding starts: the code do not use &s[0] at all!
And if .c_str() point to a shared zero-terminator buffer then capacity will be zero meaning we will not write to it before calling the Resize callback.

I think Jeremie's post established that it's guarantee to work with C++11.
As posted even though the standard says so, I find it hard to imagine an older practical implementation that would ever have the pointer returned by c_str() be different from the internal representation.

I will add a comment suggestion it's only guaranteed with C++11 and requesting feedback on pre-C++11 setups (which are very unlikely to ever be submitted.. the only people who are still working on pre-C++11 setups are the kind of code base which not using std::string).

I am modifying the header to say:

// imgui_stl.h
// Wrappers for C++ standard library (STL) types (std::string, etc.)
// This is also an example of how you may wrap your own similar types.

// Compatibility:
// - std::string support is only guaranteed to work from C++11. 
//   If you try to use it pre-C++11, please share your findings (w/ info about compiler/architecture)

// Changelog:
// - v0.10: Initial version. Added InputText() / InputTextMultiline() calls with std::string

Naming:

The problem I have with "std" ("standard") is that it is obvious in the context of C++ but imgui is not only used in a C++ context, and using the word "standard" for anyone else not in this ecosystem is misleading.

A better name would be imgui_cpp.h / imgui_cpp.cpp maybe, although the double cpp on the later filename is rather weird?

ocornut added a commit that referenced this issue Aug 31, 2018
@eliasdaler
Copy link
Contributor

the code do not use &s[0] at all!

Sorry, I was just thinking that &s[0] might be better, but it seems like c_str() is actually fine in C++11.

I think the name "imgui_cpp_std" might be even better. :) (remember libstdc++ for example)

@jdumas
Copy link
Contributor

jdumas commented Aug 31, 2018

Well if we're being picky about names then maybe imgui_stdlib.h would be more accurate then. I think the _cpp suffix is redundant, since the main ImGui library is already using C++ (otherwise people may think it's in C?). I was fine with _stl.h, but the SO answer pointed to by @eliasdaler brings some good points.

@eliasdaler
Copy link
Contributor

Agreed, imgui_stdlib is perfect.

@ocornut ocornut changed the title misc/stl/imgui_stl.h misc/cpp/imgui_stdlib.h (was: misc/stl/imgui_stl.h) Oct 12, 2018
ocornut added a commit that referenced this issue Oct 12, 2018
@ocornut
Copy link
Owner Author

ocornut commented Oct 12, 2018

I have now renamed this to misc/cpp/imgui_stdlib.h, misc/cpp/imgui_stdlib.cpp.
The misc/cpp/ folder will be a good spot to host things such as #2096 in the future.
( Tagging #1713 )

@aCuria
Copy link

aCuria commented Feb 27, 2020

The sample function InputTextCallback does not return any value, although the function pointer ImGuiInputTextCallback expects an int to be returned?

The calling code does not check the return value either.

@ocornut
Copy link
Owner Author

ocornut commented Feb 27, 2020

Hello @aCuria,
The code in misc/cpp/imgui_stdlib.cpp seems all good to me.

@aCuria
Copy link

aCuria commented Feb 28, 2020

Hello @ocornut ,

What i meant is that the sample code (InputTextCallback) you posted in this thread does not return a value:

static int InputTextCallback(ImGuiInputTextCallbackData* data)
{
    if (data->EventFlag == ImGuiInputTextFlags_CallbackResize)
    {
        // Resize string callback
        std::string* str = (std::string*)data->UserData;
        IM_ASSERT(data->Buf == str->c_str());
        str->resize(data->BufTextLen);
        data->Buf = (char*)str->c_str();
    }

    // return ? // what should be returned here?
}

It is not clear what the return value should be, and the code in ImGui that calls this callback function also does not make use of the integer return value. I was thinking that if the str->resize() function fails then perhaps InputTextCallback should return 1 to indicate that the ImGuiInputTextFlags_CallbackResize callback has failed.

One other minor detail is that you can use str->data() instead of (char*)str->c_str()

@dougbinks
Copy link
Contributor

The callback return value is only used for ImGuiInputTextFlags_CallbackCharFilter, where a value of 1 means discard.

So I would return 0.

@ocornut
Copy link
Owner Author

ocornut commented Jul 16, 2020

Yes the full code always returns 0:

https://github.com/ocornut/imgui/blob/master/misc/cpp/imgui_stdlib.cpp#L21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants