Imgui: ImGui::GetWindowSize() non constant

[andreyugolnik]

Hello,

Consider changes below:

imgui.h

IMGUI_API const ImVec2& GetWindowSize() const;

imgui.cpp

const ImVec2& ImGui::GetWindowSize() const
{
}

[ocornut]

Hello Andrey,
That doesn't make sense because ImGui is a namespace, not a class which you instantiate. All the functions are loose functions within that namespace.

[andreyugolnik]

What about return by reference instead by value?

[ocornut]

Not all those functions return values that are persistently stored somewhere (they are computed).
What are you trying to achieve with those changes?

[andreyugolnik]

As can I see, ImGui::GetWindowSize() returns ImVec2 Size field of ImGuiWindow struct, nothing more.

[ocornut]

But that would be leaking an implementation detail that may change in the future, and many similar functions don't do that.
What are you trying to achieve with those changes?

[andreyugolnik]

What are you trying to achieve with those changes?

Performance.
Rely on RVO is not good idea, I think. I mean "that it is possible to return by reference, it is better to return by reference".

[paulsapps]

ImGui has to work good in debug too where there won't be any RVO. Also ImVec2 is 8 bytes AFAIK, a reference is usually a pointer under the hood which on 64bit machines is also 8 bytes. So nothing gained.

[andreyugolnik]

As I undestand, in this case we have temp value and copy 8 bytes at least.
In reference case we have only one pointer (8 bytes on 64bit arch, as you mentioned above) without temp value and copying.

[paulsapps]

If you test you will see performance is not changed. In release both code is the same. Only debug matters.

[andreyugolnik]

Small example

struct s
{
  float x, y;
};
s Array[10];

s getByValue(unsigned idx)
{
  return Array[idx];
}

const s& getByReference(unsigned idx)
{
  return Array[idx];
}

llvm 3.8.1 -O2 https://godbolt.org/g/d69MtS
and gcc 6.3 -O2 https://godbolt.org/g/qviqBH

[ocornut]

• It is nitpicky, borderline ridiculous to care about a function called GetWindowSize() which isn't on anywhere near the critical path of any reasonable app, when thousands of other functions are traversed.
• You also choose the ignore that variety of similar functions aren't returning references and it would be odd and leaking implementation details to choose a difference signature on a per-function basis. Consistency across the API is more valuable. This type of function is a minority.
• This not a suitable test, you aren't doing anything with the values. User may want to access them multiple times, modify the values etc.
• By choosing to think about that you are choosing to not think about other more valuable things to do on the library. Ditto for me, I should focus on more important things.

[andreyugolnik]

• GetWindowSize() useful to calculate window size aligned to bottom or right side.
• I just point your view to the GetWindowSize() ;)
• It doesn't mater because GetWindowSize() get access to non constant value.
• Maybe you're right. It's your project and you boss ;)

[paulsapps]

If you care about perf look at all the comments Omar added for perf todo's and fix those.