aiekick / ImGuiFileDialog

Full featured file Dialog for Dear ImGui
MIT License
1.25k stars 202 forks source link

Assert used without including <cassert> header #169

Closed lilggamegenius closed 8 months ago

lilggamegenius commented 9 months ago

if the macro IM_ASSERT is defined inside of imconfig.h, imgui will not include the cassert header. A single assert is used here but without including cassert resulting in a undefined symbol error.

The simple fix would be to just include the header but honestly I don't see the point in catching an exception only abort the program. It leaves the program with no way actually handle the error or at least gracefully exit.

The ideal solution would be to just remove the try/catch entirely and just allow the exception to propagate up, giving some chance of handling the error.

aiekick commented 9 months ago

Hello,

this asserts is here for inform the user than his code is bad. this issue concern only the regex filter, if you fix it during the debug, you will never have another issue during software life time about that. since filters have not to be dynamics Like some assert in ImGui if you not call the ImGui::End() by ex.

exception mechanism is used here because std::regex throw him for a bad format...

but ok if can use instead the macro IM_ASSERT if you prefer.

lilggamegenius commented 9 months ago

The issue is that if the regex is user input or otherwise defined after compile-time, There wouldn't be a way to fix it before hand.
std::regex exceptions are already of type std::regex_error which already tells you your regex is bad, and not catching it also lets the IDE breakpoint on where the exception was triggered, and the callstack up to that point, making debugging easier. Assertions just abort in debug builds making it harder to debug and because its in a try/catch, release builds will simply silently catch the exception and not give any indication the regex was invalid.

aiekick commented 9 months ago

you will have the same issue if you generate dialog at runtime ans forgot to do a imgui::end

by design you have yo check than you regex are ok

lilggamegenius commented 9 months ago

Missing an imgui end function call is not easily recoverable, while passing an incorrect regex string is easily recoverable. And if the build isn't a debug build, it tells no one that it failed due to the assert being replaced with a no-op. If the exception were simply be allowed to be thrown, which in addition to allowing the exception to be caught, would also call the various destructors potentially allowing data to be saved, and also allow the problem to be noticed on both debug and release builds.

aiekick commented 8 months ago

the assert is removed, replaced by a message but the exception handling is preserved