ImGuiIntegration: Add conversion between ImStrv and StringView #96
Conversation
hugoam
force-pushed
the
imgui-stringview-convert
branch
from
8064fe3 to
a2eab62
Compare
|
Yes, absolutely! :) Couldn't find this in the main imgui branch, so I suppose it's originating from this PR, right? ocornut/imgui#3038 Can you add a bit of documentation, in a similar spirit to the other types listed above? Especially mentioning the PR/branch, and the |
|
|
||
| }}} | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
Instead of closing #ifndef DOXYGEN_GENERATING_OUTPUT here, you could just move the below #ifndef above, including the comment.
| #ifdef IMGUI_HAS_IMSTR | ||
| namespace Corrade { namespace Containers { namespace Implementation { | ||
|
|
||
| template <> struct StringViewConverter<const char, ImStrv> { |
There was a problem hiding this comment.
| template <> struct StringViewConverter<const char, ImStrv> { | |
| template<> struct StringViewConverter<const char, ImStrv> { |
Sorry, being picky :P
| #include <Magnum/Types.h> | ||
| #include <Magnum/Math/Vector.h> | ||
|
|
||
| #ifndef DOXYGEN_GENERATING_OUTPUT | ||
| #ifdef IMGUI_HAS_IMSTR |
There was a problem hiding this comment.
| #ifdef IMGUI_HAS_IMSTR | |
| /* Currently present only in the features/string_view branch of ImGui */ | |
| #ifdef IMGUI_HAS_IMSTR |
... or something like that, to make it clear why the ifdef is there.
hugoam
force-pushed
the
imgui-stringview-convert
branch
from
a2eab62 to
5588c06
Compare
hugoam
force-pushed
the
imgui-stringview-convert
branch
from
5588c06 to
e51eafc
Compare
|
Apologies for cancelling the CI, I have very little free CircleCI credits left for June and saving them for emergency reasons 🔥 If you can wait with pushing until July 1st, that'd be best. Thanks 😅 |
|
Actually, I just went and temporarily disabled PR builds on CircleCI, so that should preserve the scarce minutes, hopefully. AppVeyor could be enough to verify correctness, ImGui is tested there as well I hope. But due to CircleCI I'll likely not merge before 1st, sorry 😅 |
| #ifdef IMGUI_HAS_IMSTR | ||
| void stringView(); | ||
| #endif |
There was a problem hiding this comment.
| #ifdef IMGUI_HAS_IMSTR | |
| void stringView(); | |
| #endif | |
| #ifdef IMGUI_HAS_IMSTR | |
| void stringView(); | |
| #endif |
Indent preprocessor same as other code. Same below :)
| &IntegrationTest::colorLiterals | ||
| }); |
| CORRADE_COMPARE(StringView(imStrv), stringView); | ||
|
|
||
| ImStrv c(stringView); | ||
| CORRADE_COMPARE(c, stringView); |
There was a problem hiding this comment.
Hm, as far as I can see, this would convert c to a StringView and then use Corrade's own equality comparison. Which I don't think is wanted, since it is no different from the other comparison above.
Instead, isn't there something like ImStrCmp that could be used here?
| @@ -57,11 +57,27 @@ Example usage: | |||
| #include "Magnum/ImGuiIntegration/visibility.h" /* defines IMGUI_API */ | |||
There was a problem hiding this comment.
Update the docs above to mention the string conversion, please ;)
Also feel free to add yourself to the copyright header in the license block above the file.
| @@ -57,11 +57,27 @@ Example usage: | |||
| #include "Magnum/ImGuiIntegration/visibility.h" /* defines IMGUI_API */ | |||
|
|
|||
| #include <imgui.h> | |||
| #include <Corrade/Containers/StringView.h> | |||
There was a problem hiding this comment.
This would ideally be also wrapped in #ifdef IMGUI_HAS_IMSTR.
Alternatively I was thinking of deinlining the conversion into a new Integration.cpp file and having just a forward declaration here, but that's maybe taking it a bit too far. The StringView header is not that huge.
|
@hugoam ping ;) |
|
Sorry, didn't have much time to look at this again and then I went on a vacation. I'll try to have a look soonish, I promise |
Hello !
This one is kind of self-explanatory :)
Do you think we could merge something in this spirit?