[typeof][scope_exit] VS2017 and option /permissive-

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Hello,

late last year, VS2017RC gained a compiler option /permissive- which
disables some non-conforming behaviour. Source code that was accepted by
previous versions of msvc, and still is accepted by msvc 14.1 without
this compiler option, is rejected and flagged as an error if
/permissive- is engaged. For more on this see
https://blogs.msdn.microsoft.com/vcblog/2016/11/16/permissive-switch/.

While checking our company's codebase for possible problems related to
that I noticed compile errors in Boost.Log and traced the problem back
to Boost.Typeof and in particular
"libs\typeof\include\boost\typeof\msvc\typeof_impl.hpp". Therein is code
emulating the missing __typeof__ keyword by taking advantage of a silent
msvc compiler 'bug' as a 'feature'. Parts of this code are duplicated in
a msvc-specific workaround in Boost.Scope_Exit. This compiler bug is no
longer silent with /permissive- in place but rather a hard error. As it
turns out, I'm not the first to be bitten by this: ticket #12900 tells
the same story.

Many other Boost libraries use Boost.Typeof directly or indirectly and
therefore suffer from the same problem (like Boost.Log in my particular
case).

I've implemented a possible solution and created two pull-requests:
- https://github.com/boostorg/typeof/pull/7
- https://github.com/boostorg/scope_exit/pull/3

The patches only affect msvc 14.1. With these patches applied, the test
results of the full Boost testsuite are the same as without these
patches. I don't know enough about __typeof__ to decide if my emulation
is compatible in every aspect, but at least it's good enough to pass the
testsuite.

There are two other compile failures introduced with /permissive- in
place which are compiler bugs which need to be addressed by Microsoft's
compiler team. Fortunately these are very specific to just two
libraries, Boost.Spirit.Repository (one test case) and Boost.Geometry
(two test cases caused by one single function).

--
PGP/GPG: 2CCB 3ECB 0954 5CD3 B0DB 6AA0 BA03 56A1 2C4638C5

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Daniela Engert wrote:
...
> I've implemented a possible solution and created two pull-requests:
> - https://github.com/boostorg/typeof/pull/7

I think that a more principled solution here would be to check
!defined(BOOST_NO_CXX11_DECLTYPE) at the very start of typeof.hpp (if
BOOST_TYPEOF_EMULATION is not defined) and then use a generic decltype-based
implementation. Something like

-#if defined(__COMO__)

+#if !defined(BOOST_NO_CXX11_DECLTYPE) && !defined(BOOST_TYPEOF_EMULATION)
+#    ifndef BOOST_TYPEOF_NATIVE
+#        define BOOST_TYPEOF_NATIVE
+#    endif
+
+#elif defined(__COMO__)

(which would also require the inclusion of <boost/config.hpp>)

and then

#elif defined(BOOST_TYPEOF_NATIVE)
#   define BOOST_TYPEOF_TEXT "using native typeof"
#   include <boost/typeof/message.hpp>

-#   include <boost/typeof/native.hpp>

+#   if !defined(BOOST_NO_CXX11_DECLTYPE)
+#      include <boost/typeof/decltype.hpp>
+#   else
+#       include <boost/typeof/native.hpp>
+#   endif

where decltype.hpp is more or less the same as the suggested
msvc/typeof_impl_decltype.hpp, but without ensure_obj (shouldn't be needed
with decltype) and with remove_cv_ref instead of remove_reference.

Except that we don't have remove_cv_ref in TypeTraits, so
remove_cv<remove_reference>.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Am 15.04.2017 um 15:05 schrieb Peter Dimov via Boost:
> Daniela Engert wrote:
> ...
>> I've implemented a possible solution and created two pull-requests:
>> - https://github.com/boostorg/typeof/pull/7
>
> I think that a more principled solution here would be to check
> !defined(BOOST_NO_CXX11_DECLTYPE) at the very start of typeof.hpp (if
> BOOST_TYPEOF_EMULATION is not defined) and then use a generic
> decltype-based implementation. Something like
...
> where decltype.hpp is more or less the same as the suggested
> msvc/typeof_impl_decltype.hpp, but without ensure_obj (shouldn't be
> needed with decltype) and with remove_cv_ref instead of remove_reference.
>
> Except that we don't have remove_cv_ref in TypeTraits, so
> remove_cv<remove_reference>.

Thanks for this suggestion, Peter, I was thinking along the same lines
as well. But for a start, I rather went for the low-risk path affecting
only one compiler. During my experiments to find a fix for my problem I
noticed that the decltype-based implemention required a template type
alias to work thoughout all Boost libraries. Otherwise the compiler
would complain about either a missing 'typename' keyword or a
superfluous one. Thus, as long as there is no better way to deal with
that the first test would have to check for
!defined(BOOST_NO_CXX11_TEMPLATE_ALIASES) as well. I will implement this
in the next couple of minutes and start-off another run of the full
Boost test-suite to see what's happening...

Any comments on that?

Ciao
   Dani

--
PGP/GPG: 2CCB 3ECB 0954 5CD3 B0DB 6AA0 BA03 56A1 2C4638C5

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Daniela Engert wrote:

> Thanks for this suggestion, Peter, I was thinking along the same lines as
> well. But for a start, I rather went for the low-risk path affecting only
> one compiler. During my experiments to find a fix for my problem I noticed
> that the decltype-based implemention required a template type alias to
> work thoughout all Boost libraries. Otherwise the compiler would complain
> about either a missing 'typename' keyword or a superfluous one.

Interesting. I didn't think of that but you're right. This is probably the
reason qualifiers are stripped via a helper function and not with a type
trait in the original implementation.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Am 15.04.2017 um 16:23 schrieb Peter Dimov via Boost:

> Interesting. I didn't think of that but you're right. This is probably
> the reason qualifiers are stripped via a helper function and not with a
> type trait in the original implementation.

I've reworked my pull-request by incorporating your suggestions with
some minor additional tweaks required to make it work. The tests of
Boost.Typeof pass with msvc10, msvc12, msvc14, and msvc14.1, and a test
run of the full Boost testsuite with msvc14.1 is successful as well.
Given the generality of the suggested change, I'd appreciate if someone
would test it on gcc and clang, too.

Ciao
  Dani

--
PGP/GPG: 2CCB 3ECB 0954 5CD3 B0DB 6AA0 BA03 56A1 2C4638C5

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
Daniela Engert wrote:
> I've reworked my pull-request by incorporating your suggestions with some
> minor additional tweaks required to make it work. The tests of
> Boost.Typeof pass with msvc10, msvc12, msvc14, and msvc14.1, and a test
> run of the full Boost testsuite with msvc14.1 is successful as well.
> Given the generality of the suggested change, I'd appreciate if someone
> would test it on gcc and clang, too.

Tests pass for me on Cygwin g++ 6.3.0 (03/11/14/1z), clang++ 3.9.1
(03/11/14/1z), msvc-8.0, 10.0, 11.0, 12.0, 14.1.

> +#define BOOST_TYPEOF(expr)
> boost::type_of::remove_cv_ref_t<decltype((boost::type_of::ensure_obj(expr)))>

The ensure_obj helper shouldn't be necessary here. The above tests also pass
with

+#define BOOST_TYPEOF(expr) boost::type_of::remove_cv_ref_t<decltype(expr)>

and the definition of ensure_obj removed.

I've also enabled Travis for typeof, so any future pushes in the PR should
be CI-tested.

Tests fail with msvc-14.1 /std:c++latest because of use of binder1st,
auto_ptr, unary_function and so on, but that's a separate issue and
unrelated to these changes, although we'll probably have to fix it at some
point.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Daniela Engert wrote:

> I've reworked my pull-request by incorporating your suggestions with some
> minor additional tweaks required to make it work.

> +#if !defined(BOOST_NO_CXX11_DECLTYPE) &&
> !defined(BOOST_NO_CXX11_TEMPLATE_ALIASES)
> +#   define BOOST_TYPEOF_DECLTYPE
> +#   if !defined(BOOST_TYPEOF_EMULATION) && !defined(BOOST_TYPEOF_NATIVE)
> +#       define BOOST_TYPEOF_NATIVE
> +#   endif
> +#endif
> +
> #if defined(__COMO__)

I'd think that if we hit the decltype case, we no longer need to enter the
compiler-specific branches below that. So

#if !defined(BOOST_NO_CXX11_DECLTYPE) &&
!defined(BOOST_NO_CXX11_TEMPLATE_ALIASES) &&
!defined(BOOST_TYPEOF_EMULATION)
#   define BOOST_TYPEOF_DECLTYPE
#   if !defined(BOOST_TYPEOF_NATIVE)
#       define BOOST_TYPEOF_NATIVE
#   endif

#elif defined(__COMO__)

perhaps? Tested as before.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Am 15.04.2017 um 23:52 schrieb Peter Dimov via Boost:
> Daniela Engert wrote:
>
> Tests pass for me on Cygwin g++ 6.3.0 (03/11/14/1z), clang++ 3.9.1
> (03/11/14/1z), msvc-8.0, 10.0, 11.0, 12.0, 14.1.

Great - thanks!

>> +#define BOOST_TYPEOF(expr)
>> boost::type_of::remove_cv_ref_t<decltype((boost::type_of::ensure_obj(expr)))>
>>
>
> The ensure_obj helper shouldn't be necessary here. The above tests also
> pass with

I remember seeing failures without that in former test runs, but that
might have been caused by other missing pieces.

> I've also enabled Travis for typeof, so any future pushes in the PR
> should be CI-tested.

Thanks a lot, Travis is 'terra incognita' to me.

> Tests fail with msvc-14.1 /std:c++latest because of use of binder1st,
> auto_ptr, unary_function and so on, but that's a separate issue and
> unrelated to these changes, although we'll probably have to fix it at
> some point.

Well, all my tests with msvc-14.0 and msvc-14.1 run with /std:c++latest.
This engages different, c++17-related code paths which already exhibited
an error in one Boost library (fixed now). And test runs with msvc-14.1
have /permissive- attached from now on.

Ciao
   Dani

--
PGP/GPG: 2CCB 3ECB 0954 5CD3 B0DB 6AA0 BA03 56A1 2C4638C5

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [typeof][scope_exit] VS2017 and option /permissive-

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Am 16.04.2017 um 00:11 schrieb Peter Dimov via Boost:

> I'd think that if we hit the decltype case, we no longer need to enter
> the compiler-specific branches below that. So
>
> #if !defined(BOOST_NO_CXX11_DECLTYPE) &&
> !defined(BOOST_NO_CXX11_TEMPLATE_ALIASES) &&
> !defined(BOOST_TYPEOF_EMULATION)
> #   define BOOST_TYPEOF_DECLTYPE
> #   if !defined(BOOST_TYPEOF_NATIVE)
> #       define BOOST_TYPEOF_NATIVE
> #   endif
>
> #elif defined(__COMO__)
>
> perhaps? Tested as before.

True. The pull-request is reworked along that line and I like the result
of that much more than my original code. It has the additional benefit
that it can be easily rebased or cherry-picked onto master (like I do
with my take on Boost 1.64).

My tests with msvc pass, and Travis is happy with it as well.

Ciao
   Dani

--
PGP/GPG: 2CCB 3ECB 0954 5CD3 B0DB 6AA0 BA03 56A1 2C4638C5

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Loading...