On ImGuiAxis_None

[VeganPower]

Version/Branch of Dear ImGui:

Version 1.92.0 Branch: master

Back-ends:

custom back end.

Compiler, OS:

CLang 21.1.7

Full config/build information:

Not relevant. The issue is triggered by coverity static analysis.

Details:

Hello,
this may seem a bit as a pet peeve, so I'll try to justify.

While working on a project I got some warning from static analysis in places where ImGuiAxis is used as index to access a ImVec2.

Like here:

void ImGui::NavClearPreferredPosForAxis(ImGuiAxis axis)
{
    ImGuiContext& g = *GImGui;
   // Static analysis reported a interger overflow warning here.
    g.NavWindow->RootWindowForNav->NavPreferredScoringPosRel[g.NavLayer][axis] = FLT_MAX;
}

After a quick check I notice that ImGuiAxis can assume the values None (-1) X (0) or Y(1).. and is always used as ImGuiAxis_X and ImGuiAxis_Y.
And at least on the master branch and in the version I'm using (1.92.0) I can't find a single use of ImGuiAxis_None.
So I dismissed the static analysis warning as "false positive".. but I wanted to investigate a bit more.

In the snipped here: https://gcc.godbolt.org/z/cYnd4fqPd

With
#define MY_ISSUE 1
#define BAD_IDEA 0
and you compile with "-O2 -DNDEBUG"

You should get the following ASM as output.

foo(ImVec2 const&, ImGuiAxis):
        movsx   rsi, esi                       // sign extension 
        movss   xmm0, DWORD PTR [rdi+rsi*4]    // memory deference at [this + axis * 4]
        ret

The sign extension is required because ImGuiAxisis a signed type.
If you remove ImGuiAxis_None (comment it or set MY_ISSUE 0) the enum is converted to unsigned and the output should be something like this.

foo(ImVec2 const&, ImGuiAxis):
        movss   xmm0, DWORD PTR [rdi+rsi*4]    // memory deference at [this + axis * 4]
        ret

That someone may call "It's just one clock cycle less." but I call it "100% speed increase."
I told you it was a pet peeve.

But in general I can't justify why ImGuiAxis has a value of ImGuiAxis_None = -1, is never used (AFAIK), it trigger static analysis warning and add (one) extra instruction.

The same thing happen in DEBUG, but in this case the assert check are the one that add extra instructions.
If you compile with "-O2 -D_DEBUG" you get something like this:

.LC0:
        .string "float ImVec2::operator[](size_t) const"
.LC1:
        .string "/app/example.cpp"
.LC2:
        .string "idx == 0 || idx == 1"
foo(ImVec2 const&, ImGuiAxis):
        movsx   rsi, esi  <--- Sign extension.
        cmp     rsi, 1
        ja      .L7
        movss   xmm0, DWORD PTR [rdi+rsi*4]
        ret
.L7:
        push    rax
        mov     ecx, OFFSET FLAT:.LC0
        mov     edx, 32
        mov     esi, OFFSET FLAT:.LC1
        mov     edi, OFFSET FLAT:.LC2
        call    __assert_fail

The part below is probably a bad idea.. so feel free to dismiss it.

If you define a special operator[] overload that accept a ImGuiAxis you can skip the check as Axis can only be 0 or 1 (apart from previous UB.. in that case you have a bigger problem somewhere else), like foo((ImGuiAxis)512).
But it's probably a bad idea because ImGuiAxis is in imgui_internal.h and imVec2 is in the public interface. You can add enum class to get some extra warning.. but this is C++, you can shoot in your foot if you really want. So paying the extra cost for the assert is probably better.

I added it to the snippet just for ASM comparison. Defining BAD_IDEA 1 you get the same ASM in release and debug.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

https://gcc.godbolt.org/z/cYnd4fqPd

#include <cstdlib>
#include <assert.h>

#define IM_ASSERT(_EXPR) assert(_EXPR)

#define MY_ISSUE 1
#define BAD_IDEA 0

#if MY_ISSUE
enum ImGuiAxis
{
    ImGuiAxis_None = -1,
    ImGuiAxis_X = 0,
    ImGuiAxis_Y = 1
};
#else
enum ImGuiAxis : size_t
{
    ImGuiAxis_X = 0,
    ImGuiAxis_Y = 1
};
#endif


struct ImVec2
{
    float                                   x, y;
    constexpr ImVec2()                      : x(0.0f), y(0.0f) { }
    constexpr ImVec2(float _x, float _y)    : x(_x), y(_y) { }
    float& operator[] (size_t idx)          { IM_ASSERT(idx == 0 || idx == 1); return ((float*)(void*)(char*)this)[idx]; } // We very rarely use this [] operator, so the assert overhead is fine.
    float  operator[] (size_t idx) const    { IM_ASSERT(idx == 0 || idx == 1); return ((const float*)(const void*)(const char*)this)[idx]; }
#if BAD_IDEA
    // Enum can't be (without UB) anything other than 0 or 1
    float  operator[] (ImGuiAxis idx) const { return ((const float*)(const void*)(const char*)this)[(size_t)idx]; }
#endif
#ifdef IM_VEC2_CLASS_EXTRA
    IM_VEC2_CLASS_EXTRA     // Define additional constructors and implicit cast operators in imconfig.h to convert back and forth between your math types and ImVec2.
#endif
};

float foo(ImVec2 const& x, ImGuiAxis a)
{
    return x[a];
}