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

TextUnformatted implementation does not match expected behavior for its interface #3615

Closed
Xeverous opened this issue Nov 24, 2020 · 3 comments

Comments

@Xeverous
Copy link

Version/Branch of Dear ImGui:

Version: 1.80 WIP (17906)
Branch: master (71cc636)

Back-end/Renderer/Compiler/OS

Back-ends: https://github.com/mosra/magnum-integration/tree/master/src/Magnum/ImGuiIntegration
Compiler: all
Operating System: all

My Issue/Question:

I'm very surprised by implementation's behavior.

const std::string_view text = /* ... */;
ImGui::TextUnformatted(text.data(), text.data() + text.size());

The code above crashes in some cases. Why? Because the implementation does not match what library user (me in this case) expects the library to do.

void TextUnformatted(const char* text, const char* text_end);

As someone who is working with idiomatic C++, this function takes a range. A range can be defined by an iterator + size or 2 iterators. Pointers are the simplest types that are iterators. There is an invariant that end iterator (which should be equal to begin + size) can not be dereferenced.

The problem: I'm passing a valid range. In my case, the string is empty and the range is (ptr, ptr + 0). ptr == nullptr but this should be irrelevant - what is relevant here is that begin == end and end is not dereferencable, therefore ptr should not be dereferenced whatever it is. Entire STL works with empty ranges and so any other library may be expected to honor range idioms.

I guess it will be very easy to fix. I just want to point out that not "honoring" range idiom here is very surprising considering how the interface looks. I was very pleased by seeing an interface that does not require null-terminated strings and it was very tempting to use it in some STL/boost algorithms with a lambda that calls Dear ImGui.

@ocornut
Copy link
Owner

ocornut commented Nov 25, 2020

The problem is that accepting TextUnformatted(0, 0) as a valid call will prevent a detecting a common error. I believe that may be more problematic.

@Xeverous
Copy link
Author

Xeverous commented Nov 25, 2020

What common error do you have in mind? 0 (first), 0 (size) or 0 (first), 0 (last) are both completely valid ranges and ... an empty non-null-terminated string is still a valid string. Any function/algorithm that traverses arrays is expected to work with empty ranges. Often no extra code needs to be written to handle empty ranges because they just immediately terminate all possible loops. In Dear ImGui's case it's only failing because the condition for null-terminated string is at the beginning which is not aware that the given string is a valid non-null-terminated string.

@ocornut
Copy link
Owner

ocornut commented Aug 24, 2021

Added support for this with 780c1ee.
Sorry for late reaction and thanks for the suggestion.
Also added basic tests for it.

What common error do you have in mind? 0 (first), 0 (size) or 0 (first), 0 (last) are both completely valid ranges

They are valid when intended when passed explicitly as a range (two params), but consider that second parameter is defaulting to NULL:

ImGui::TextUnformatted(SomeValue())

When SomeValue() return NULL, you'd expect standard text functions to helpfully crash on a NULL pointer as we are often doing for being a low-level library. Instead this is now treated as a NULL,NULL range and will silently display an empty line. It is debatable whether this is desirable.

But simultaneously I already the desired support for ranges is nice but I opted for it.

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

2 participants