New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support msvc #6
Comments
it's at line 1100 of qglobal.h |
Thanks for having a look at that. Do you have any patches ? As for 3., ad @jcelerier said, it is in qglobal.h. Could it be that __cpp_variable_templates is notdefined properly with MSVC? Maybe something we also need to fix in Qt itself to be able to use qOverload Regarding 4, you might be right. Depending if MSVC allows that. |
re: 3 Since Qt is using this method of feature detection as well, I guess they just fail to define |
Re 2: It seems that it can be relativaly easily worked around by introducing an intermediary variable template. I have this patched, I'll upload soon. |
@mwu-tow that would be nice! I'm trying to use it in https://github.com/dolphin-emu/dolphin/ , so will test with that. On (3) again: I wonder if Qt knows that msvc doesn't actually support the feature test macros they are using...? |
Re 2: Workaround is here: mwu-tow@9fd3e2b Re 3: It seems to be a Qt issue, MSVC does not support feature test macros. There should be test for MSVC version, assuming that MSVC's variable template implementation can handle the code. |
Re 3: Report posted here: https://bugreports.qt.io/browse/QTBUG-61667 As for 1 — it seems it was once reported to MSVC and closed as not a bug. See the report mentioned in the linked SO question: https://connect.microsoft.com/VisualStudio/feedback/details/676585/bug-in-cl-c-compiler-in-correct-expansion-of-va-args |
For the preprocessor issue, I think it would be good to report it either way, although I'm pretty doubtful it will get fixed judging by historical responses. |
Re 3:
I find such choice — seemingly hostile towards MSVC users — hard to understand. |
@mwu-tow with these changes: https://github.com/shuffle2/verdigris/commit/e553801100424d1217a5873b948475d709aef28f void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title); msvc generates this:
and these errors:
The other verdigris code i'm using so far ( Haven't looked into this Edit: Commenting out the offending Edit 2: With the help of @booto , narrowed it down to this repro: https://godbolt.org/g/as6fPA Now to see if there's a decent way around, etc... Edit 3: @booto figured out that swapping the order which |
So, after the above workaround, it seems the last blocker (at least for dolphin's usage, which is just void RequestStop() W_SIGNAL(RequestStop); to output: void RequestStop() {
using w_SignalType = decltype((&W_ThisType::RequestStop)); <-- incorrectly formed?
return w_internal::SignalImplementation<w_SignalType, w_signalIndex_RequestStop__34>{this}();
}
static constexpr int w_signalIndex_RequestStop__34 = decltype(w_SignalState(w_internal::w_number<>{}, static_cast<W_ThisType**>(nullptr)))::size;
friend constexpr auto w_SignalState(w_internal::w_number<w_signalIndex_RequestStop__34 + 1> w_counter, W_ThisType **w_this) ->
decltype(w_internal::binary::tree_append(
w_SignalState(w_counter.prev(), w_this),
w_internal::makeMetaSignalInfo(
(&W_ThisType::RequestStop), <-- incorrectly formed?
"RequestStop" <-- missing comma
w_internal::makeStaticStringList("","","","","","","","","","","","","","","","") <-- missing comma
w_internal::makeStaticStringList("","","","","","","","","","","","","","","",""))))
{
return w_internal::binary::tree_append(
w_SignalState(w_counter.prev(), w_this),
w_internal::makeMetaSignalInfo(
(&W_ThisType::RequestStop), <-- incorrectly formed?
"RequestStop" <-- missing comma
w_internal::makeStaticStringList("","","","","","","","","","","","","","","","") <-- missing comma
w_internal::makeStaticStringList("","","","","","","","","","","","","","","","")));
}; Edit: This behavior is actually explicitly documented on msdn, so it's unlikely to change: https://docs.microsoft.com/en-us/cpp/preprocessor/variadic-macros In any case, the real compiler errors seem to be worked around, now. Just need to rewrite some of the |
Instead of putting the SD-6 workaround in verdigris, I'm intending to put it in the using project: dolphin-emu/dolphin#5721 @ogoffart mergable commits to workaround the msvc bugs can be found here https://github.com/shuffle2/verdigris/commit/cd9c544c140c5142ef0cfc2d299daf62eebd65f8 (from @mwu-tow but without the SD-6 change) and here https://github.com/shuffle2/verdigris/commit/070925898c6b93172d2ce22e99822eeee305c258 I also split out the |
Thanks everyone for your effort. I have already merged two commits from shuffle2. I have not merged the one with |
I tried to play a little more with the preprocessor issue. One of the issues is that It is as if You can interactively change the code from the link above and observe the preprocessed results both in MSVC and Clang. I hope it might be useful. If someone knows what is the proper way to work around this issue, please write here. |
Stupid me. |
Did more experiments: With the above fixes I can some |
This should now be fixed by ed35968 |
Add some required macro expentions, and work around a couple of issues Note, qmake needs to be run with an extra argument: DEFINES+="__cpp_constexpr=201304 __cpp_variable_templates=201304" so that the C++14 defines in qt are active. Also, the tests inside the qt durectory have class with too many invokable method and it reaches the recursion limit Issue: #6
Good news, with commit 064747e, it somehow works. Still need to add Also, the tests tst_qobject and tst_qmetaobject reaches the recursion limit or use too much memory. So we would potentially need another way to create the string array for MSVC. Maybe using some C++17 features it is possible to improve compilation time. Testers welcome. |
I've been trying a bit to make verdigris work with Qt 5.9 + msvc (vs2017 update 2).
Known bug: https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly with known workaround (see
#define EXPAND
)The second issue I found is that apparently msvc cannot handle evaluating functions within template parameters which are passed parameter packs. i.e. The following code fails because msvc doesn't deal with
sums(Flags...)
correctly:If you could find another way to write this, it would be great! (I'm not a template wizard). I'll try to report the issue to msvc anyways. Note that the code still fails on the Update 3 preview build.
qOverload was not defined. It looks like you intended for this to be in mainline Qt5.7, but it's not found in my Qt5.9 install. Maybe I'm just doing something wrong?
Currently there is some code that expects any windows build to mean MingW. That should probably be changed. https://github.com/woboq/verdigris/blob/master/src/wobjectimpl.h#L746
The text was updated successfully, but these errors were encountered: