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

New issue

Closed

opened 2020-11-24 21:02:39 +00:00 by Xeverous · 3 comments

Xeverous commented

2020-11-24 21:02:39 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

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

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.

**Version/Branch of Dear ImGui:** Version: 1.80 WIP (17906) Branch: master (71cc636696bd17c81514da49707e909958c14632) **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. ```cpp 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. ```cpp 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.

👍 3

ocornut commented

2020-11-25 18:26:43 +00:00

(Migrated from github.com)

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.

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 commented

2020-11-25 20:20:30 +00:00

(Migrated from github.com)

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.

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 commented

2021-08-24 17:36:27 +00:00

(Migrated from github.com)

Added support for this with 780c1ee2.
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.

Added support for this with 780c1ee2. 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.