Skip to content
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

Closed
shuffle2 opened this issue Jun 26, 2017 · 18 comments
Closed

Support msvc #6

shuffle2 opened this issue Jun 26, 2017 · 18 comments

Comments

@shuffle2
Copy link
Contributor

shuffle2 commented Jun 26, 2017

I've been trying a bit to make verdigris work with Qt 5.9 + msvc (vs2017 update 2).

  1. Known bug: https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly with known workaround (see #define EXPAND)

  2. 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:

template<typename F, int N, typename ParamTypes, int... Flags>
constexpr MetaMethodInfo<F, N, sums(Flags...) | W_MethodType::Slot.value, ParamTypes>
makeMetaSlotInfo(F f, StaticStringArray<N> &name, const ParamTypes &paramTypes, W_MethodFlags<Flags>...)
{ return { f, {name}, paramTypes, {} }; }

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.

  1. 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?

  2. 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

@jcelerier
Copy link
Collaborator

qOverload was not defined

it's at line 1100 of qglobal.h

@ogoffart
Copy link
Member

Thanks for having a look at that. Do you have any patches ?
I have been trying before but did not continue because issue 1. and 2. (I mentionned the bug with variadic template in a comment, but never reported it: https://www.reddit.com/r/cpp/comments/56c69i/in_case_youve_missed_it_visual_c_now_has_c14/d8ombku/ )
There are probably ways to work it around but i did not investigate further.

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.

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jun 26, 2017

re: 3
Ah yea, I forgot that msvc also had issues with verdigris' use of __cpp_constexpr. It seems that this manner of feature detection is supported in VS' IDE (i.e. by intellisense) - but not supported by the compiler. It's a bit confusing, but there are two different compilers in use...
In any case, msvc at least claims to support constexpr properly, it just don't support the feature detection :/

Since Qt is using this method of feature detection as well, I guess they just fail to define qOverload while they really should be able to.

@mwu-tow
Copy link

mwu-tow commented Jun 27, 2017

Re 2:
I have already reduced it and reported to MS: https://developercommunity.visualstudio.com/content/problem/23768/cannot-expand-variadic-template-parameter-pack-in.html
If you want, you may upvote the report to increase its visibility.

It seems that it can be relativaly easily worked around by introducing an intermediary variable template. I have this patched, I'll upload soon.
However I have not performed any proper testing other than checking that header compile.

@shuffle2
Copy link
Contributor Author

@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...?

@mwu-tow
Copy link

mwu-tow commented Jun 27, 2017

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.
I'll report this to Qt.

@mwu-tow
Copy link

mwu-tow commented Jun 27, 2017

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
Haven't read into details but if their reasoning is valid, then non-standard GCC-compatible construct should be used. Otherwise they should be bothered with more reports. Reportedly they have revamping the preprocessor in plans, it might be good for them to know that such issue bothers us and hampers adoption of MSVC into cross-platform libraries. What do you think?

@shuffle2
Copy link
Contributor Author

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.

@mwu-tow
Copy link

mwu-tow commented Jun 27, 2017

Re 3:

Thiago Macieira added a comment - 10 minutes ago
Not our bug, by choice. Please convince Microsoft to set the __cpp_variable_templates macro.

I find such choice — seemingly hostile towards MSVC users — hard to understand.

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jun 28, 2017

@mwu-tow with these changes: https://github.com/shuffle2/verdigris/commit/e553801100424d1217a5873b948475d709aef28f
and this example:

void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);

msvc generates this:

void RequestTitle(const QString& title) {
    using w_SignalType = decltype(qOverload<const QString&>(&W_ThisType::RequestTitle));
    return w_internal::SignalImplementation<w_SignalType, w_signalIndex_RequestTitle__33>{this}(title);
  }
  static constexpr int w_signalIndex_RequestTitle__33 = decltype(w_SignalState(w_internal::w_number<>{}, static_cast<W_ThisType**>(nullptr)))::size;
  friend constexpr auto w_SignalState(w_internal::w_number<w_signalIndex_RequestTitle__33 + 1> w_counter, W_ThisType **w_this) ->
    decltype(w_internal::binary::tree_append(
        w_SignalState(w_counter.prev(), w_this),
        w_internal::makeMetaSignalInfo(
            qOverload<const QString&>(&W_ThisType::RequestTitle),
            "RequestTitle",
            w_internal::makeStaticStringList("const QString&","","","","","","","","","","","","","","",""),
            w_internal::makeStaticStringList("title","","","","","","","","","","","","","","",""))))
        {
            return w_internal::binary::tree_append(
                w_SignalState(w_counter.prev(), w_this),
                w_internal::makeMetaSignalInfo(
                    qOverload<const QString&>(&W_ThisType::RequestTitle),
                    "RequestTitle",
                    w_internal::makeStaticStringList("const QString&","","","","","","","","","","","","","","",""),
                    w_internal::makeStaticStringList("title","","","","","","","","","","","","","","","")));
        };

and these errors:

1>C:\src\dolphin\Source\Core\DolphinQt2/Host.h(33,43): error C2752: 'w_internal::SignalImplementation<w_SignalType,0>': more than one partial specialization matches the template argument list
1>  void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);
1>                                          ^
1>C:\src\dolphin\Source\Core\DolphinQt2/Host.h(33,43): note: could be 'w_internal::SignalImplementation<Ret(__cdecl Obj::* )(Args...),Idx>'
1>  void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);
1>                                          ^
1>C:\src\dolphin\Source\Core\DolphinQt2/Host.h(33,43): note: or       'w_internal::SignalImplementation<void(__cdecl Obj::* )(Args...),Idx>'
1>  void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);
1>                                          ^
1>C:\src\dolphin\Source\Core\DolphinQt2/Host.h(33,43): error C2440: 'initializing': cannot convert from 'initializer list' to 'w_internal::SignalImplementation<w_SignalType,0>'
1>  void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);
1>                                          ^
1>C:\src\dolphin\Source\Core\DolphinQt2/Host.h(33,43): note: Invalid aggregate initialization
1>  void RequestTitle(const QString& title) W_SIGNAL(RequestTitle, (const QString&), title);
1>                                          ^

The other verdigris code i'm using so far (W_OBJECT, W_OBJECT_IMPL, W_SLOT) seems to work.

Haven't looked into this W_SIGNAL error yet, have you already encountered it?

Edit: Commenting out the offending SignalImplementation overload (and thus not making the compiler choose which to pick) allows it to compile. So...is this another compiler bug, or something else?

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 Obj and Ret appear in the template parameter list allows msvc to compile...cough

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jun 28, 2017

So, after the above workaround, it seems the last blocker (at least for dolphin's usage, which is just W_OBJECT, W_OBJECT_IMPL, W_SLOT, W_SIGNAL) is that msvc's preprocessor also doesn't like when __VA_ARGS__ holds zero parameters. This causes e.g. this:

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 __VA_ARGS__ usage to support msvc (which would have been required to be portable, anyways).

@shuffle2
Copy link
Contributor Author

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 EXPAND change here https://github.com/shuffle2/verdigris/commit/bf84cb76c258e5c9c5182eeb3c883adf38338799 but i'm not sure this should be merged yet, since it seems the macro stuff will need to be rewritten a bit to deal with msvc not allowing 0 args to variadic macros. Any chance you want to look into that? :>

ogoffart added a commit that referenced this issue Jul 3, 2017
@ogoffart
Copy link
Member

ogoffart commented Jul 3, 2017

Thanks everyone for your effort. I have already merged two commits from shuffle2.

I have not merged the one with EXPAND yet as there is still issues with it from what i understand. (Also, please name it W_EXPAND).
I might look at it at some point if i find time to try MSVC again.

@mwu-tow
Copy link

mwu-tow commented Jul 18, 2017

I tried to play a little more with the preprocessor issue. One of the issues is that W_OVERLOAD_REMOVE evaluates to nothing MSVC. Unfortunately I don't have much experience with preprocessor trickery and I'm not sure what exactly is wrong. I tried to reduce the issue step by step and got the following:

https://godbolt.org/g/2E9yNp

It is as if W_MACRO_DELAY2 did not expand the W_MACRO_TAIL.

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.

@mwu-tow
Copy link

mwu-tow commented Jul 18, 2017

Stupid me. EXPAND, as suggested before, does the trick for this one. Will check what are the remaining issues.

@mwu-tow
Copy link

mwu-tow commented Jul 18, 2017

Did more experiments:
a) EXPAND has also to be applied on W_PARAM_TOSTRING — otherwise when called for (a,b) we get ("a, b", "", "", "", "", "", "",…. Also, I applied it to W_OVERLOAD_RESOLVE by introducing additonal macro level, so it is not necessary to correct each call.
b) there is a problem with MSVC erasing the trailing comma if variadic pack was empty in cases like #NAME, W_PARAM_TOSTRING(W_OVERLOAD_TYPES(__VA_ARGS__)). It can be worked around by wrapping W_PARAM_TOSTRING(W_OVERLOAD_TYPES(__VA_ARGS__)) in an additional pair of parentheses — so the comma is not the last character before macro.

With the above fixes I can some W_SIGNAL usage compiling (along with W_OBJECT and W_OBJECT_IMPL). However, W_SLOT and W_INVOKABLE remain broken. When I have a little more time, I'll try to give it another push.

ogoffart added a commit that referenced this issue Jan 4, 2018
…he ...

Fix warnings reported in #7
Might help for MSVC support: #6
@ogoffart
Copy link
Member

ogoffart commented Jan 4, 2018

msvc's preprocessor also doesn't like when VA_ARGS holds zero parameters.

This should now be fixed by ed35968

ogoffart added a commit that referenced this issue Jan 10, 2018
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
@ogoffart
Copy link
Member

Good news, with commit 064747e, it somehow works.

Still need to add DEFINES+="__cpp_constexpr=201304 __cpp_variable_templates=201304" as a qmake argument. I guess I could add it in some .pri file

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants