Refactor in 1.64 (imgui_widgets.cpp)

[ocornut]

Here's an idea I have and I would happily get your feedback on it.

Aiming to release 1.63 shortly (maybe this week).

Immediately after I will release 1.64 which would ONLY consist in a shuffling code around.

Some of the code in imgui.cpp would be split into imgui_widgets.cpp. As you can guess imgui_widgets.cpp will contains all the widgets specific code, whereas imgui.cpp would contains the core.
In my preliminary tests, about 30-35% of imgui.cpp would move to imgui_widgets.cpp.
(I expect that % to grow as down the line I'd like to remove stb_textedit.h and re-implement a custom version, as much of our InputText() issues are tricky to solve with the current code)

Behavior shouldn't be affected but moving code around will break local forks and make them conflicts when merged. Making a clear documented transition between 1.63->1.64 will facilitate the update for people with non-trivial fork (they will be advised to first update to 1.63, isolate their diffs, then update to 1.64 and reapply).

This would be a change analogous to 1.44 when I extracted imgui_draw.cpp out of imgui.cpp.
This would obviously break the build for project which have an hardcoded list of file instead of imgui*.cpp but the fix should be trivial.

I will also perform various performance tests (e.g. how do non-static functions in a same compilation unit affect optimizations, if any? does anyone has info to share about that for msvc/clang/gcc?) and depending on the result will aim at including the most touched low-level function calls called from widgets into imgui_widgets.cpp if it makes a difference.

While doing that shuffle I will take the occasion to probably re-order many functions in both imgui.cpp and imgui_widgets.cpp, and section some areas with comments. (exact sectioning/ordering criteria unknown, but I'll aim to make it easier for people to locate code. e.g. maybe one of the criteria for imgui_widgets.cpp might be that the function ordering matches the ordering in imgui.h?)

[bkaradzic]

This might help: https://stackoverflow.com/a/8131212

[thedmd]

I mean:

template <>
extern IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

extern should be after template. Otherwise it will try to reefer to extern templates which were removed from C++.

Edit: this is probably bad advice

[ocornut]

This works with Clang/Mingw:

.h

extern template 
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

.cpp

template
float ImGui::RoundScalarWithFormat<float,float>(const char* format, ImGuiDataType data_type, float v);

in the .cpp file with C+11.

VS2010 fails with:

1>imgui_internal.h(1253): warning C4231: nonstandard extension used : 'extern' before template explicit instantiation
1>imgui_widgets.cpp(1510): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

In spite of https://msdn.microsoft.com/en-us/library/hh567368.aspx saying that Extern Template are supported in VS2010.

VS2012 fails with:

imgui_widgets.cpp(1511): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

Both VS2010/VS2012 errors because imgui_widgets.cpp both sees the extern and the instantiation.. If you add an ifdef block to avoid the extern declaration when compiled from imgui_widgets.cpp it works, but we'd mess up with unity builds?

VS2015 compiles/links OK.

[thedmd]

Found a solution:

imgui_internal.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template
IMGUI_API float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

imgui_widgets.cpp

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v)
{
    const char* fmt_start = ImParseFormatFindStart(format);
    if (fmt_start[0] != '%' || fmt_start[1] == '%') // Don't apply if the value is not visible in the format string
        return v;
    char v_str[64];
    ImFormatString(v_str, IM_ARRAYSIZE(v_str), fmt_start, v);
    const char* p = v_str;
    while (*p == ' ')
        p++;
    if (data_type == ImGuiDataType_Float || data_type == ImGuiDataType_Double)
        v = (TYPE)ImAtof(p);
    else
        ImAtoi(p, (SIGNEDTYPE*)&v);
    return v;
}

// instantiations
template
IMGUI_API float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

This should work. When template <> means specialization, template means instantiation. Today I le-learned.

[ocornut]

With your solution (only adding ImGui:: to the implementations in the .cpp file)

VS2010 is OK

Clang

In file included from main.cpp:5:
../..\imgui_internal.h:1253:29: error: explicit instantiation of undefined function template 'RoundScalarWithFormat'
    IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);
                            ^
../..\imgui_internal.h:1250:29: note: explicit instantiation refers here
    IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

(from every calls)

Similar stuff from Mingw

In file included from main.cpp:5:
../../imgui_internal.h: In instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]':
../../imgui_internal.h:1253:117:   required from here
../../imgui_internal.h:1253:117: error: explicit instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' but no definition available [-fpermissive]
     IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);
                                                                                                                     ^
../../imgui_internal.h: In instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]':
../../imgui_internal.h:1253:117:   required from here
../../imgui_internal.h:1253:117: error: explicit instantiation of 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' but no definition available [-fpermissive]
../../imgui_internal.h:1250:29: warning: inline function 'TYPE ImGui::RoundScalarWithFormat(const char*, ImGuiDataType, TYPE) [with TYPE = float; SIGNEDTYPE = float; ImGuiDataType = int]' used but never defined
     IMGUI_API inline TYPE   RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);
                             ^~~~~~~~~~~~~~~~~~~~~
I

[bkaradzic]

Oh C++ templates... Keep jiggling it, it's going to work eventually... :)

[thedmd]

Could you try to remove instantiation from header?
imgui_internal.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API inline TYPE RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

VS swallowed that.

[jdumas]

Ok so you actually don't need any extern or inline keyword, and should be able to separate the implementation if you use explicit template instantiation (which is different from explicit template specialization, but the syntax can be confusing).

Minimal example

foo.h

#pragma once

// Function declaration
template<typename T> void show(T x);

foo.cpp

#include "foo.h"
#include <iostream>

// Function definition
template<typename T> void show(T x) {
	std::cout << x << std::endl;
}

// Explicit template instantiation
template void show<int>(int);

main.cpp

#include "foo.h"

int main(void) {
	show((int) 1);
	return 0;
}

Compilation

clang++ -c foo.cpp
clang++ -c main.cpp
clang++ main.o foo.o 

--> No warning whatsoever.

In any case, if you have a generic implementation of a templated function that you want to make available to everyone, but want to keep separation between header files and implementation, I recommend to 1) put the (forward) declaration of the templated function in imgui_internal.h, 2) put the definition (implementation) of the templated function in a separate file imgui_internal.tpp, and 3) #include "imgui_internal.tpp" at the end of imgui_internal.h.

PS: It's actually template-fu (as in kung-fu), not template-foo, but as a programmer one can argue about the legitimity of this second form :D

[ocornut]

@thedmd

Could you try to remove instantiation from header?

That's equivalent to my initial problem. Works with VS 10/12/15, Clang warns.

---

@jdumas

Ok so you actually don't need any extern or inline keyword, and should be able to separate the implementation if you use explicit template instantiation (which is different from explicit template specialization, but the syntax can be confusing).
No warning whatsoever

Minus the Explicit template instantiation (which is not strictly required in this case since there is an instantiation with the same type, but I'll leave it, better be explicit) - this is what my first/current version is doing, but Clang warns with -Weverything. The warning happens on the calling site, so we can't even easily disable them.

---

Perhaps I'm missing something but at this point I don't have an ideal solution.
If some of you want to try it, it would be good to test with at least VS2010, VS2015 and Clang with high-warning settings.

The worry if that we have a weak point here that would trigger warning/error on esoteric compilers.

To clarify a few points:

(Clang)
.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE          RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

main.cpp:28:22: warning: instantiation of function 'ImGui::RoundScalarWithFormat<float, float>' required here, but no
      definition is available [-Wundefined-func-template]
    float f = ImGui::RoundScalarWithFormat<float,float>("%.1f", ImGuiDataType_Float, 1.123f);

---

Explicit instantiation seems to require knowing of the implementation in the compilation unit:
(Clang)
.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE   RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template
float   RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

imgui_internal.h:1254:13: error: explicit instantiation of undefined function template 'RoundScalarWithFormat'
    float   RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

---

Explicit specialization requires a second implementation of the function:
(Clang)
.h

template<typename TYPE, typename SIGNEDTYPE>
IMGUI_API TYPE  RoundScalarWithFormat(const char* format, ImGuiDataType data_type, TYPE v);

template<>
float RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

imgui_widgets-b6fa7c.o : error LNK2001: unresolved external symbol "float __cdecl ImGui::RoundScalarWithFormat<float,float>(char const *,int,float)" (??$RoundScalarWithFormat@MM@ImGui@@YAMPBDHM@Z)
a.exe : fatal error LNK1120: 1 unresolved externals

---

CURRENT SOLUTION I FOUND

Using extern template AT THE CALL SITE

extern template
IMGUI_API float         RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

The problem is that this block cannot be included in imgui_internal.h, as it disable the instantiation of the template in imgui_widgets.cpp, so the function instance ends up missing when linking.

Now if I also explicitly instantiate in the .cpp file the Clang is happy but not VS:
imgui_widgets.cpp:

template
float ImGui::RoundScalarWithFormat<float, float>(const char* format, ImGuiDataType data_type, float v);

1>..\..\imgui_widgets.cpp(1510): error C2929: 'float ImGui::RoundScalarWithFormat<float,float>(const char *,ImGuiDataType,float)' : explicit instantiation; cannot explicitly force and suppress instantiation of template-class member

Which can be worked out with some define/ifdef but that could break the possibility of using unity builds?

---

TL;DR;

For now I can offer to declare the templates in imgui_internal.h.
The user will need to use an extern template in their .cpp file in order to silence Clang warning, or silence clang warnings.

[ocornut]

@bkaradzic As stated above I have pushed changes to declare the template function in imgui_internal.h
You'll need an extern template statement in your range_slider code to use them.

I also renamed them:

Renamed RoundScalarWithFormat<> -> RoundScalarWithFormatT<>
Renamed SliderBehaviorCalcRatioFromValue<> -> SliderCalcRatioFromValueT<>

Let me know if that solves your issue!

[jdumas]

Hmm if the warning comes from clang's -Weverything then I vote for ignoring it. -Weverything enables really all warnings, even those that are considered experimental, undesirable, or lead to false positive. It does not necessarily makes sense to heed those warning all the time, and there's a good reason not all warnings make it to -Wall and -Wextra. Now, in this case, I see this message as a way to warn against potential linker errors for someone that doesn't know what they're doing and forget to instantiate or define the necessary template function in other parts of the code. But this really defeats the purpose imho and sounds like a false positive you can safely ignore.

Honestly template function are just like macros that the compiler will expand and implement depending on what type is used. If the function body is available, the compiler will compile the function in every translation unit where it is used. But if only the declaration is available, then it will issue a standard function call and stuff will be resolved at linking time. It's just like any other function, which do not need an available definition to be called properly.

My recommendation: don't mess with extern qualifiers, it's probably not needed here, especially if it's only to make clang's -Weverything happy. I'll also throw a modified version of my example above that makes clang's -Weverything happy, without any extern keyword:

foo.h

#pragma once

// Declaration of the generic template function
template<typename T> void show(T x);

// Declaration of the specialized function
template<> void show(int x);

foo.cpp

#include "foo.h"
#include <iostream>

namespace {

template<typename T> void show_gen(T x) {
	std::cout << x << std::endl;
}

}

// Definition of the specialized function
template<> void show<int>(int x) {
	show_gen(x);
}

main.cpp

#include "foo.h"
#include <iostream>

int main(void) {
	show(int(1));
	return 0;
}

As you can see, it's mostly doing some gymnastic to avoid a warning that shouldn't be there in the first place.

[ocornut]

Then I vote for ignoring it.

I agree but as stated I cannot ignore it as the warning happens on the call site, so it's up to @bkaradzic there.

[bkaradzic]

@ocornut it builds with GCC, still waiting other compilers to finish:

https://github.com/bkaradzic/bgfx/blob/2904b42dafdcae37dfbda260dff543fb53e9faa0/3rdparty/dear-imgui/widgets/range_slider.inl#L7

[bkaradzic]

GCC 5 linker only complains about SliderCalcRatioFromValueT but not RoundScalarWithFormatT even thought both have <float, float> internal instantiation.

../../linux64_gcc/bin/libexample-commonRelease.a(imgui.o): In function `ImGui::RangeSliderBehavior(ImRect const&, unsigned int, float*, float*, float, float, float, int, int)':
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:117: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:131: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'
/home/travis/build/bkaradzic/bgfx/.build/projects/gmake-linux/../../../3rdparty/dear-imgui/widgets/range_slider.inl:131: undefined reference to `float ImGui::SliderCalcRatioFromValueT<float, float>(int, float, float, float, float, float)'

[ocornut]

@bkaradzic I'm not sure I understand why, sorry. Copying your extern template" and making a dummy call to SliderCalcRatioFromValueT()works for me with Mingw 8.1 and 5.1 (they both link properly). Because of howSliderBehavior()` is setup with a switch statement on all data type the instantiation should always happen.

[bkaradzic]

Ok it's wasn't GCC 5 specific (it happens with GCC 7 too), just in release build.

I had to do this for now... Until I figure out why it's getting stripped:
https://github.com/bkaradzic/bgfx/blob/350e5d45c3639c98566b033a8c8ca967a8d4ff84/3rdparty/dear-imgui/widgets/range_slider.inl#L13

[ocornut]

Yeah that looks strange to me (looking at bkaradzic/bgfx@350e5d4#diff-3307f41d1c9cdf8632e3d8fdb3fa320f)

Closing this now, looks like it's solvable in user land with those declaration.
It's worth thinking about those issues for then I'll put more serious work into improving imgui_internal.h to allow building custom widgets.

[bkaradzic]

Also I wouldn't mind if you implement that range slider as part of ImGui. :)

[ocornut]

Will do, #76 is still open :)