Fix issues with Windows and the Interconnect library about corrade HOT 8 CLOSED

That's very unfortunate and leads to nasty hard-to-prevent bugs.

Do you mean bugs in Corrade itself or in an application using Corrade::Interconnect? What a typical bug might look like with regard to this issue?

from corrade.

In the application. It'll result in a differently named signal being called, triggering completely different slots from what you'd expect.

from corrade.

What do you think about CORRADE_EMIT() macro workaround, but with a __FUNCSIG__ as a source of uniqueness?

__FUNCSIG__ (as well as __FUNCDNAME__) gives a full function signature, with substituted template arguments - if a function is templated.

from corrade.

Interesting, didn't know about __FUNCSIG__ / __FUNCDNAME__, thanks! Yes, this could probably work, however it probably will cause binary bloat as the function signatures will get embedded in the binary (mangled signatures are slightly shorter, but still), unless they get hashed or something, in which case it'll be a big compile-time disadvantage, I'm afraid.

The main problem is I don't like the macro :( If I compare to /OPT:NOICF (which also might cause binary bloat due to duplicates being present), forcing /OPT:NOICF on library users seems like a better idea.

from corrade.

The main problem is I don't like the macro :(

Who likes macros? But what if a macro had a bonus - by making signal declarations less verbose, something like CORRADE_TSIGNAL(Class, Signal, TemplateType, (int, std::string...))? I actually don't know whether such thing is possible, may be Boost Preprocessor has a recipe...

… compare to /OPT:NOICF (which also might cause binary bloat due to duplicates being present) …

Which variant takes more memory cannot be said for sure until measured and most likely the result will vary from application to application. Also, probably the macro can be optimized to use __FUNCDNAME__ only for templated signals and __COUNTER__ otherwise.

… forcing /OPT:NOICF on library users seems like a better idea.

Some will interpret such a restriction as a library defect!

from corrade.

I don't think the current signal declarations are verbose, quite the contrary. Using a macro like you suggest, in my opinion, would make things overly opaque and prevent signal name/signature autocompletion in "less gifted" IDEs. Moreover, the current design allows for extra flexibility -- turning references into copies, having default arguments for signals etc.

I see /OPT:NOICF as a known-to-be-working workaround for a non-conforming compiler (as per the reddit link above) -- which, in my book, is a defect on the compiler side, not on the library side. I still hope the workaround can be done in a less intrusive way. The reddit thread suggests using a function-local static variable. Another solution, similar to __FUNCDNAME__ but without the macro requirement, could be using std::source_location as a default argument to emit() (also see #55).

from corrade.

Finally got around to fixing this:

• 0ce8cb4 extends the test to verify the actually problematic cases
• f8b7e1c and a5b0dcb fixes issues with signals implemented in classes with multiple inheritance
• and 5bfa63a explicitly disables /OPT:ICF for all users of the Interconnect library. I tried really hard to find some other solution that would be both reliable and not extremely annoying to use, but failed, so this has to do. Related docs.

Regarding MinGW, everything in the tests was "just working" with the current state, so I assume all works correctly there. If not, that'll get handled in a separate issue, since problems described here are fairly MSVC-specific.

from corrade.

Regarding MinGW, everything in the tests was "just working" with the current state, so I assume all works correctly there.

In case anybody is still subscribed to this issue: I managed to reproduce and test for the MinGW-specific bug in #72, the Ui library has a fix workaround for that now as well.

from corrade.