ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
61.18k stars 10.31k forks source link

va_copy() with GCC and old c++ standards #1085

Closed stefanos82 closed 6 years ago

stefanos82 commented 7 years ago

First of all, congrats for this amazing project; it really looks both awesome and promising.

Now, as the title says, which C++ version is supported by default?

I'm asking, because I took a look at the code and looks like a hybrid of C89 with C++98.

Can someone enlighten me please?

Thank you.

ocornut commented 7 years ago

It's either C++98 or C++03 I'm not even sure I know. (Definitively not using C++11). I'm been only testing as far as Visual Studio 10 lately. If you have specific requirements maybe test with your compilers?

stefanos82 commented 7 years ago

It's alright. I will experiment around it and see how it goes.

By the way, if you want to add an extra layer of security around NULL, you can implement your own version that is really the same as nullptr, but for C++98/C++03. Let's call it nil:

const class {
    // (private by default in classes)
    // not allowed to get the address
    void operator&() const;

public:
    template<typename T>
    operator T*() const
    {
        return 0; 
    }

    template<typename T, typename C>
    operator T C::*() const
    {
        return 0;
    }
    operator void*() const
    {
        return 0;
    }
} nil = {};

That's it, really. Now, you can use it in place of NULL and have the same security as nullptr.

Cheers.

P.S.: I don't quite remember which one of the two C++ gurus first implemented this; either is Herb Sutter's job or Scott Meyers' job; anyhow, it's working flawlessly.

ocornut commented 7 years ago

Thanks! It'd be good to know. I haven't looked into the differences between C++98 and C++03. I suspect in practice it probably doesn't matter. C++11 and further are out of usable boundaries because not supported by many minor/obscure platforms.

So far we haven't had problem with the behavior of NULL. It's not really the coding style of imgui to use template magic when the behavior might not be obvious to the casual reader so I think I'm happy with using NULL as it is, but thank you for the suggestion. That trick is good to know.

stefanos82 commented 7 years ago

The use of aforementioned nilcan be exactly the same with NULL without the slightest difference. The real magic happens behind the scenes by the compiler itself, but nevertheless you can of course use whatever it works best with the project.

Is there anywhere a list, something like wiki page or something alike that lists the currently supported platforms?

I'm always interested in learning new technologies and systems that exist out there to help me broaden my technical horizons.

ocornut commented 7 years ago

There's no wiki page of that sort. The list of supported platforms is basically any platform that can compile the code, since the library has little dependency. It has been used on all sort of video game consoles and embedded devices. Some can render it themselves, some can send the triangles back to the network to another machine for rendering.

stefanos82 commented 7 years ago

That is good to know.

Regards to the patreon sponsoring, I was thinking to share my point of view which is what I have noticed happening for more than a year now with various projects out there.

Based on various FOSS-ed projects that I have been reviewing every now and then, they have received a plethora of suggestions by companies that are currently using those projects and all of them asked from those projects the same thing: a business invoice.

As a company they have stated, they are willing to pay for a product, but as a donation, they are hesitating to do so for some reason, which my guess is that it has something to do with taxes and legality overall.

My suggestion would be, why not follow the SQLite business model? Just take a look how they run their business and frankly, they are doing great based on the powerful clients they are currently supporting I must admit.

SQLite Pro Support

I think this concept is the ideal for dear imgui and I'm sure companies would love to pay for professional support.

The best thing you can do is to send an email to companies that already use it and kindly ask for valuable feedback around this idea.

I'm more than sure they will gladly say a loud yes.

stefanos82 commented 7 years ago

After much consideration and a bit of extra experimentation around my nullptr implementation, I would like to withdraw my suggestion and kindly request to continue using the existing syntax dear imgui is already using.

My only worry with NULL is when you use it in an overloaded function that accepts integers and pointers. That's the case you should really use C++11's nullptr to avoid ambiguity.

void foo(int);
void foo(int *);                   
void foo(char *);
void foo(const char *);

void foo(NULL); // Which one should it call? NULL is 0 in C++ for backwards compatibility with C
codz01 commented 7 years ago

why should some one call a function with null pointer as an argument !!? .

stefanos82 commented 7 years ago

@codz01 There are cases where such function includes a printf() in it and the user might want to validate the function's calling and of course NULL is a valid input wherever a pointer is taking place.

ocornut commented 7 years ago

Closing this idle topic. As @stefanos82 is the person interested in knowing version of C++ between 98 and 03 is required and said he would look at it, if you do please post the result here, would be nice :) Thanks!

stefanos82 commented 7 years ago

@ocornut Both flags -std=c++98 and -std=c++03 when used with GCC compiler, the following error is thrown:

../../imgui.cpp: In member function ‘void ImGuiTextBuffer::appendv(const char*, __va_list_tag*)’:
../../imgui.cpp:1605:24: error: incompatible types in assignment of ‘__va_list_tag*’ to ‘va_list {aka __va_list_tag [1]}’
     va_copy(args_copy, args);
                        ^
../../imgui.cpp:1598:36: note: in definition of macro ‘va_copy’
 #define va_copy(dest, src) (dest = src)
                                    ^~~
Makefile:58: recipe for target '../../imgui.o' failed
make: *** [../../imgui.o] Error 1

When -pedantic flag is used though along with std=gnu++98 or -std=c++11 and so forth, the following warnings are thrown:

warning: ISO C forbids conversion of object pointer to function pointer type [-Wpedantic]

So, in other words any flag that is greater or equally compatible with C99 will work as expected.

As we know by the standards, C++11 became compatible with C99 for backwards compatibility and C++14 with C11. Now about C++17 I don't know yet what kind of compatibility they will apply, we shall wait and see until June or July.

In other words, we need further feedback around this and if indeed it depends on C++11 for compilation, then my suggestion would be to fully implement the code in C++11 to take advantage many new and useful features that would improve code both in readability and in a more secured way.

Your opinion, let alone your objective though it has nothing to do with my naive, yet sincere suggestion.

Cheers.

ocornut commented 7 years ago

That va_copy error is not really a C++ version related error, more like a "GCC/glibc support of C++ versions" error. Annoyingly va_list is defined __va_list_tag [1] in the libc header file, so the va_list parameters and the local variable using va_list can't be just copied to each others. See http://stackoverflow.com/questions/8047362/is-gcc-mishandling-a-pointer-to-a-va-list-passed-to-a-function

I don't know how to fix it yet. Probably won't try to fix it until an actual real user needs it, rather than a hypothetical use case of testing for various flags you wouldn't actually use.

stefanos82 commented 7 years ago

Allow me to disagree with you @ocornut. It definitely is a C++ version related error.

As you can see from this link http://en.cppreference.com/w/c/variadic/va_copy, you can see it says (since C99) and by using a C++ flag that is less than C++11 -- which is compatible with C99 by the ANSI / ISO standards -- it means the least version used with dear imgui should be C++11 or gnu++98, flag-speaking if we go with GCC compiler that is, to avoid producing such error messages.

ocornut commented 7 years ago

I wonder if va_copy exists as a function (not a macro) with your settings where is fails, and therefore we are redefining it. Does removing the block that defines a default va_copy works with those settings?

stefanos82 commented 7 years ago

I didn't edit any kind of code; all I did was to run both OpenGL examples inside examples directory you provide with dear imgui and got these results.

ocornut commented 7 years ago

That's why I am asking if removing the block that default a default va_copy in imgui.cpp would fix it that with configuration?

stefanos82 commented 7 years ago

Can you please be more specific?

Did you mean to comment out the following lines

#ifndef va_copy
#define va_copy(dest, src) (dest = src)
#endif

that are located in lines 1597 to 1599 or did you mean something else?

Your enlightenment around this topic would be of great help.

stefanos82 commented 7 years ago

@ocornut OK, I have managed to work around it, but I have only tested it with GCC and Clang.

You just need to replace va_copy(args_copy, args); in line 1607 of imgui.cpp with __builtin_va_copy(args_copy, args); if you want and will compile just fine with C++98.

But let's make things simpler as they should have been in the first place:

#if defined(__GNUC__) || defined(__clang__)
#undef va_copy
#define va_copy(dest, src) __builtin_va_copy(dest, src)
#endif

Place this piece of code just below #ifndef va_copy that you wrote for versions prior to Visual Studio 2013 and your code should work as expected with all sorts of C++ versions.

Besides, that is what /usr/lib/x86_64-linux-gnu/6/include/stdarg.h it's implementing for C99 and C++11 as you can see for yourself:

...

#if !defined(__STRICT_ANSI__) || __STDC_VERSION__ + 0 >= 199900L \
    || __cplusplus + 0 >= 201103L
#define va_copy(d,s)    __builtin_va_copy(d,s)
#endif
#define __va_copy(d,s)  __builtin_va_copy(d,s)
...

So basically we are "hacking" around the standards so to speak to accomplish a backwards compatibility with old C++ standard, like that of C++98.

ocornut commented 7 years ago

Thank you for digging it. Those things are particularly tricky to get right for every compilers/lib/versions combo indeed.

Flix01 commented 7 years ago

Mainly OT, but if you want imgui to be further more portable...

Compiling with Visual C++ Toolkit 2003 (it was the first Microsoft compiler with dot Net support, the only one that Microsoft released as a command-line package at the time (not now)), I get:

../../imgui.cpp(973) : error C3861: 'vsnprintf': identifier not found, even with argument-dependent lookup
../../imgui.cpp(984) : error C3861: 'vsnprintf': identifier not found, even with argument-dependent lookup
../../imgui.cpp(1607) : error C3861: 'vsnprintf': identifier not found, even with argument-dependent lookup

that can be fixed simply with something like:

#ifdef _MSC_VER
#if (!defined(vsnprintf))
#   define vsnprintf _vsnprintf
#endif //if (!defined(vsnprintf))
#endif //_MSC_VER

P.S. I've assumed vsnprintf is always defined as a macro (when it's present) in the code above. Otherwise we could make deeper version checks:

MSVC++ 14.1 _MSC_VER == 1910 (Visual Studio 2017)
MSVC++ 14.0 _MSC_VER == 1900 (Visual Studio 2015)
MSVC++ 12.0 _MSC_VER == 1800 (Visual Studio 2013)
MSVC++ 11.0 _MSC_VER == 1700 (Visual Studio 2012)
MSVC++ 10.0 _MSC_VER == 1600 (Visual Studio 2010)
MSVC++ 9.0  _MSC_VER == 1500 (Visual Studio 2008)
MSVC++ 8.0  _MSC_VER == 1400 (Visual Studio 2005)
MSVC++ 7.1  _MSC_VER == 1310 (Visual Studio 2003)
MSVC++ 7.0  _MSC_VER == 1300
MSVC++ 6.0  _MSC_VER == 1200
MSVC++ 5.0  _MSC_VER == 1100

[EDIT:] vsnprintf is present in imgui_demo.cpp as well.

stefanos82 commented 7 years ago

@Flix01 I have no idea how Microsoft is implementing its compiler(s), but what I know for sure is that it took them years to partially support many C99 features and still it's not fully compliant. Now why they do such thing it's beyond my understanding.

@ocornut Remember when I asked for a wiki page with all compilers being used to compile / test dear imgui?

Now it's a good time to get one so we can see which compilers support what builtin flags.

@ocornut off topic: I have sent a pull request about something you might be interested to look at https://github.com/ocornut/imgui/pull/1093

Flix01 commented 7 years ago

@stefano82 Actually from the cpp reference it seems that vsnprintf has a version before C99: int vsprintf( char *buffer, const char *format, va_list vlist ); (until C99)

So @ocornut was probably right to assume that every compiler should have it defined.

But you're right too, about "things beyond understanding" in Microsoft compilers.

stefanos82 commented 7 years ago

@Flix01 true, but in http://en.cppreference.com/w/c/io/vfprintf we can see the same version implemented with an extra keyword added to stream and format, named restrict which this keyword is not implemented in C++ by the standards, still though compilers such as GCC, Clang, and Visual C++ have workarounds:

In other words, I firmly believe dear imgui should be C99 / C++11 compliant as it eliminates certain restrictions from the developer and the compiler, plus C99 it's supported on a large range of devices if we take into consideration the Linux kernel that depends on C99 / GNU99 flags.

What do you think @ocornut ?

Flix01 commented 7 years ago

@stefanos82

I firmly believe dear imgui should be C99 / C++11 compliant

I don't agree: I think broader compiler support is the best choice. (But I'm not @ocornut)

stefanos82 commented 7 years ago

@Flix01 I didn't say I disagree with you.

It would be nice though if we could take advantage of so many useful C++11 features that help the developer to produce cleaner, safer, and much more readable code than the macro madness coding style we can see in legacy code.

If that is the case, then the project should be rewritten in C89 and forget about C++'s convenience.

We are in the middle of 2017 and we should be looking forward, not backwards.

The majority of IoT devices are already using Node.js that is implemented in C++11 and that alone is an evidence that C++11 is used in multiple platforms.

I understand your concerns though and I can pretty much empathize with you.

MrSapps commented 7 years ago

I wonder who uses ImGui that needs old C++? Perhaps people using on things like PS3? What C++ version does PS3 SDK support?

stefanos82 commented 7 years ago

@ocornut Remember when I asked for a wiki page with all compilers being used to compile / test dear imgui?

Now it's a good time to get one so we can see which compilers support what builtin flags.

@paulsapps As you can see, I have quoted this for you to show that I have asked @ocornut more or less what you are asking about old C++.

I think it's something dear imgui needs to have - as a resource I mean - to know where the development should get directed the most.

It reminds me an old proverb of ours that approximately translates to "follow the flow where the money leads". That is what project's development motto should be.

Flix01 commented 7 years ago

My last post here (it's a kind of discussion where everybody can have good reasons to defend his position, so it can be never ending).

stefanos82 commented 7 years ago

My last post here (it's a kind of discussion where everybody can have good reasons to defend his position, so it can be never ending).

  • Dear Imgui is a two cpp files library. It makes sense to have the broadest possible compiler compability as a priority in my opinion.

Agreed on this point.

  • People can already use C99/C++11 in the code that uses Dear ImGui.

That is something I didn't know as I have recently found out Dear ImGui, thank you for letting me know.

  • There is a certain amount of the macro madness, but it's not that much of a mess.

The less the better. If it's inevitable...oh well ¯\_(ツ)_/¯

  • I really don't understand why we should raise the requirements of these two files.

I could ask the same kind of question for all these countless programming languages and frameworks that pop up all over the place like mushrooms that fix not a single problem, other than pleasing original creators' ego and pride.

  • I'm not sure if maintainers of ImGui ports in other languages (e.g. C#,python,Lua) will be happy with it.

Is it something @ocornut should really care about? That is, does Dear ImGui depends on these projects to function?

It sound harsh, but that is what Linus is ranting about if we compare it with Linux's development.

If a project maintainer starts hearing each and everyone's demands and wants, the project will never get fixed nor will it ever reach stability, let alone becoming feature-complete.

If Dear ImGui wants to stay with C++98 standards, no problem. But if it depends on lower level code, like that of C99's va_copy, then I would say it would be better to go with C++11 since it is backwards compatible with C99.

That is just my own opinion and I do tend to firmly support my believes; it doesn't mean I am correct nor that anyone should follow my path or my way of thinking.

I know that through constructive criticism and academic interest, incredible things can happen.

franciscod commented 6 years ago

+1, was trying to build with -std=c++03 as per a suggestion on #1962 and hit the error on both GCC/clang.

ocornut commented 6 years ago

@franciscod I tested with mingw64 under Windows and g++ -std-c++03 worked, Clang gave other errors but not something related to va_copy().

Could you 1) post the log, and 2) try to replace the va_copy definition block with:

// On some platform vsnprintf() takes va_list by reference and modifies it.
// va_copy is the 'correct' way to copy a va_list but Visual Studio prior to 2013 doesn't have it.
#ifndef va_copy
#if defined(__GNUC__) || defined(__clang__)
#define va_copy(dest, src) __builtin_va_copy(dest, src)
#else
#define va_copy(dest, src) (dest = src)
#endif
#endif

And see if it fixes it?

ocornut commented 6 years ago

@stefanos82 I pushed a va_copy fix but I couldn't find a way to repro the error you mentioned here: https://github.com/ocornut/imgui/issues/1085#issuecomment-296302826 Can you still repro it? I tried with GCC and Clang under Windows, 64-bits with -std=c++03

@franciscod It's committed now so if you can confirm the fix it would be ideal.

@Flix01 Committed a change for _vsnprintf() under Visual Studio. I'm not super sure about it but I guess if it causes other problems we'll find out.

stefanos82 commented 6 years ago

@ocornut Those warnings were taking place under GNU / Linux Debian testing 64-bit.

With the latest commit, no more warnings for me.

I have checked it with both GCC 8.1.0 and Clang 6.0.1 and works flawlessly.

Thank you @ocornut :+1:

franciscod commented 6 years ago

Works here too, GCC 8.1.1, clang 6.0.1.

ocornut commented 6 years ago

Wonderful, closing this!