C++ conformance fixes (MSVC /permissive-) #4914

Merged
merged 2 commits into from Feb 16, 2017

Projects

None yet

4 participants

@Rastaban
Contributor

We (the Microsoft C++ team) use the dolphin project as part of our "Real world code" tests.
I noticed a few issues in windows specific code when building dolphin with the MSVC compiler
in its conformance mode (/permissive-). For more information on /permissive- see our blog
https://blogs.msdn.microsoft.com/vcblog/2016/11/16/permissive-switch/.

These changes are to address 3 different types of issues:

  1. Use of qualified names in member declarations

    struct A {
         void A::f() { } // error C4596: illegal qualified name in member declaration
                         // remove redundant 'A::' to fix
    };

  2. Binding a non-const reference to a temporary

    struct S{};
      
    // If arg is in 'in' parameter, then it should be made const.
    void func(S& arg){}
      
    int main() {
       //error C2664: 'void func(S &)': cannot convert argument 1 from 'S' to 'S &'
       //note: A non-const reference may only be bound to an lvalue
       func( S() );
       
      //Work around this by creating a local, and using it to call the function
       S s;
       func( s );
    }

  3. Add missing #include <intrin.h>

Because of the workaround you are using in the code you will need to include
this. This is because of changes in the libraries and not /permissive-

@Rastaban Rastaban C++ conformance fixes (MSVC /permissive-)
We (the Microsoft C++ team) use the dolphin project as part of our "Real world code" tests.
I noticed a few issues in windows specific code when building dolphin with the MSVC compiler
in its conformance mode (/permissive-).  For more information on /permissive- see our blog
https://blogs.msdn.microsoft.com/vcblog/2016/11/16/permissive-switch/.

These changes are to address 3 different types of issues:

1) Use of qualified names in member declarations

    struct A {
        void A::f() { } // error C4596: illegal qualified name in member declaration
                        // remove redundant 'A::' to fix
    };

2) Binding a non-const reference to a temporary

    struct S{};
  
    // If arg is in 'in' parameter, then it should be made const.
    void func(S& arg){}
  
    int main() {
      //error C2664: 'void func(S &)': cannot convert argument 1 from 'S' to 'S &'
      //note: A non-const reference may only be bound to an lvalue
      func( S() );
   
      //Work around this by creating a local, and using it to call the function
      S s;
      func( s );
    }

3) Add missing #include <intrin.h>

Because of the workaround you are using in the code you will need to include
this.  This is because of changes in the libraries and not /permissive-
2ed61b0
@@ -147,7 +147,7 @@ void PathConfigPane::BindEvents()
Bind(wxEVT_UPDATE_UI, &WxEventUtils::OnEnableIfCoreNotRunning);
}
-void PathConfigPane::OnISOPathSelectionChanged(wxCommandEvent& event)
+void PathConfigPane::OnISOPathSelectionChanged(const wxCommandEvent& event)
@lioncash
lioncash Feb 16, 2017 Member

It would be preferable in this instance to change line 190 in this source file to

wxCommandEvent dummy_event{};
OnISOPathSelectionChanged(dummy_event);

instead of making the parameters const, as wxWidgets expects non-const reference parameters for event handler functions.

@Rastaban
Rastaban Feb 16, 2017 Contributor

Thank you, I can update that.

@lioncash

Overall it looks fine other than that one minor issue pointed out.

Thanks for the fixes, by the way =)

@Rastaban Rastaban wxWidgets expects non-const
revert my previous change to these files and instead create a named temporary.
34b0b1b
@Rastaban
Contributor

Sure, Thank you for letting us use your project for our tests :)

@phire phire merged commit d847986 into dolphin-emu:master Feb 16, 2017

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@Orphis
Contributor
Orphis commented Feb 16, 2017 edited

Thanks @Rastaban ! We plan on trying VS2017 as soon as the RTM is available and /permissive- was definitely a flag I was looking forward to using!
If you have found more configuration flag that are recommended and we aren't using, feel free to let us know!

@Rastaban
Contributor

Glad to hear it! There are a few conformance issues in the released version of the Windows SDK that will impact your project when building with /permissive-, so you probably won't be able to use the switch for all of your build yet. I am working with the Windows folks to get those resolved in a future SDK release. I encourage you to use it where you can though.

@Orphis
Contributor
Orphis commented Feb 16, 2017

Well, it's the Windows SDK, it's not unexpected! It preprocesses in a weird way.
I think the biggest issue we have with it are the macro conflicts when Windows provides some Unicode or Ansi alternative through a unique name. Then some 3rd party code (wxWidgets) will have functions with the same name and cause issues since they are renamed... See: https://github.com/dolphin-emu/dolphin/blob/4c004b6dc978d7585bf0765e22351ef1edd39a3a/Externals/wxWidgets3/include/wx/msw/winundef.h

Do you know if the Windows SDK team is aware of the issue or has said they would eventually fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment