search

boostorg / range

Boost.org range module
http://boost.org/libs/range
43 stars 104 forks source link

Error C4596: 'difference_type': illegal qualified name in member declaration when compiling range/concepts.hpp with MSVC141 #66

Closed ncook-hxgn closed 5 years ago

ncook-hxgn commented 6 years ago

Hi there,

I've been compiling boost 1.66.0 with MSVC 141 (Visual Studio 2017 15.5.6). When I do, I get Error C4596: 'difference_type': illegal qualified name in member declaration. This is caused by the namespace prefix at line 259 of concepts.hpp. (I'm assuming it's not a Visual Studio bug) BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n;

I've created a pull request, #65, with what I think is a fix, it works for me and passes CI. Please let me know if the fix is suitable/appropriate. If it is, is there any chance it will make boost 1.67 ?

Thanks,

Nathan

DanielaE commented 6 years ago

@ncook-vero , @neilgroves , this change causes lots of compile failures in my runs of the test suite. For details see my comments to #65

sav-ix commented 6 years ago

Hello, everyone,

For builds using MSVC 2017 15.5.0 got errors:

compile-c-c++ bin.v2\libs\date_time\build\msvc-14.1\debug\address-model-64\threading-multi\gregorian\greg_month.obj

    call "bin.v2\standalone\msvc\msvc-14.1\address-model-64\architecture-x86\msvc-setup.bat"  >nul
 cl /Zm800 -nologo @"bin.v2\libs\date_time\build\msvc-14.1\debug\address-model-64\threading-multi\gregorian\greg_month.obj.rsp" 

greg_month.cpp
c:\libBOOST-1.68.0-dev\build\boost/range/concepts.hpp(257): error C3646: 'n': unknown override specifier
c:\libBOOST-1.68.0-dev\build\boost/range/concepts.hpp(264): note: see reference to class template instantiation 'boost::range_detail::RandomAccessIteratorConcept<Iterator>' being compiled
c:\libBOOST-1.68.0-dev\build\boost/range/concepts.hpp(257): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
...failed compile-c-c++ bin.v2\libs\date_time\build\msvc-14.1\debug\address-model-64\threading-multi\gregorian\greg_month.obj...

Reproduced after PR 795046f8fc30e359d0a4826cd5bb652d3ccf3855 and f1906e914eaaf76408a0a4e69c61c457c08abba8 were merged.

A workaround:

diff --git a/libs/range/include/boost/range/concepts.hpp b/libs/range/include/boost/range/concepts.hpp
index 6fef2ea..de36a22 100644
--- a/libs/range/include/boost/range/concepts.hpp
+++ b/libs/range/include/boost/range/concepts.hpp
@@ -253,7 +253,7 @@ namespace boost {
              }
          private:
  // MSVC 14.1 - avoid C4596: 'difference_type': illegal qualified name in member declaration
- #if defined(_MSC_VER) && _MSC_VER >= 1912 
+ #if defined(_MSC_FULL_VER) && _MSC_FULL_VER > 191225830
              BOOST_DEDUCED_TYPENAME difference_type n;
  #else
              BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n;

fixed it for me. In case it would be considered as a solution, a condition _MSC_FULL_VER > 191225830 should be updated in order to correspond MSVC version, which introduced this issue.

DanielaE commented 6 years ago

Did you bother to check with current (or even still-to-be-released daily) versions of MSVC? According to my tests, at least every version beginning with 19.13 (i.e. VS 15.6) and later fails to compile with the PR in place. So, only a few 15.5.x releases require the workaround from the PR. Therefore, it's application must be restricted to this short range, i.e. #if defined(_MSC_FULL_VER) && _MSC_FULL_VER > 191225830 && _MSC_FULL_VER < 191300000 it the proper constraint.

sav-ix commented 6 years ago

Didn't check later MSVC versions yet. Assuming 15.5.6, mentioned in 1st post, should be fine.

pdimov commented 6 years ago

typename difference_type n; is ill-formed; it might be better to use something correct instead. Perhaps typename RandomAccessIteratorConcept<Iterator>::difference_type n; or typename BidirectionalIteratorConcept<Iterator>::difference_type n;

ncook-hxgn commented 6 years ago

I do like the idea of doing it correctly, apologies for my ill-formed solution - possibly it wasn't the solution I thought it was.

Of your suggestions, I prefer typename RandomAccessIteratorConcept<Iterator>::difference_type n; as it seems like the smallest change and doesn't introduce the question of what a BidirectionalIteratorConcept is to my mind - I wasn't trying to change or improve anything, I just wanted it to compile.

Are you guys doing other work on this version of boost for this compiler version? Is it worth doing, or am I distracting you from more useful things?

TL;DR: Should we just upgrade our boost to 1.67 (which says it's fine with VS2017) ?

pabristow commented 6 years ago

I have this problem on develop branch recently updated and current VS 15.7.2 typename RandomAccessIteratorConcept::difference_type n; and BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n; both work for me, FWIW.

jzmaddock commented 6 years ago

Can someone please fix this? It completely breaks Multiprecision apart from anything else.

sav-ix commented 6 years ago

+1 to something should be done at last. After moving to MSVC 2017 15.7.3 got errors:

.\boost/range/concepts.hpp(257): error C3646: 'n': unknown override specifier
.\boost/range/concepts.hpp(264): note: see reference to class template instantiation 'boost::range_detail::RandomAccessIteratorConcept<Iterator>' being compiled
.\boost/range/concepts.hpp(257): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int

A possible solution:

How's that, @ncook-vero ?

ncook-hxgn commented 6 years ago

Hi @sav-ix, we (Vero) have now upgraded to VS 2017 15.7.4, so we should be fine with your suggestion if we can build boost with it in it's unadultered-by-myself form - we prefer that really..

Essentially, I think @DanielaE hit the nail on the head: "Therefore, it's application must be restricted to this short range". You could either fix what appears to be an edge case, or advise upgrading build tools. (BTW, FYI, we did need the fix under VS 2017 15.6.6)

I know what I'd do - advise upgrade and keep clean code :)

Let me build a Boost 1.67 without my changes with VS 15.7.4, and I shall get back to you ASAP.

pabristow commented 5 years ago

I'm confused. Is Peter Dimov wrong when he says the original was ill-formed? Sounds unlikely but this is above my paygrade? I'd like the current VS at the time of release of 1.68 to work with the current VS version at that release time (and to work with the then current develop branch too of course). It's a show stopper for Multiprecision (and other ?)

pdimov commented 5 years ago

The original was fine; the fix was ill-formed (which is why it doesn't work now.)

ncook-hxgn commented 5 years ago

@pdimov @sav-ix - I still experience this with VS 2017 15.7.4. Please see screenshot below demonstrating this. Please excuse my poor redactions of our closed source.

image

However, my ill-formed fix continues to be a fix for this, for us at least, it would appear.

pdimov commented 5 years ago

Do you have a small program I can try?

ncook-hxgn commented 5 years ago

It's not very small I'm afraid, and it's rather closed source. I'll see if I can put together a reproduction. outside of our product. I'm also juggling a V8 upgrade and various rebases today. I will get back to you A.S.A.P

Edit: It seems the offending line of code in our product that is causing this error is the following function call to boost/geometry. I did actually raise an issue with boost::geometry who sent me here in the first place.

boost::geometry::get_turns<false, false, boost::geometry::detail::overlay::assign_null_policy>(inner, outer, strategy_inner_outer, robust_policy, turns, policy);

I'm not sure I'll be able to post an example outside of our production code, the code calling the fn (that when compiled is causing the problem) is rather maths-heavy, and alas, I am not. I shall see if I can reproduce it with that one line somehow..

jzmaddock commented 5 years ago

However, my ill-formed fix continues to be a fix for this, for us at least, it would appear.

Well this is very strange, because for me with a fresh install of 15.7.4 and a program that consist of just:

#include <boost/range/concepts.hpp>

I get the:

1>c:\data\boost\boost\boost\range\concepts.hpp(257): error C3646: 'n': unknown override specifier

error.

This is debug, x64, no pre-compiled headers.

pdimov commented 5 years ago

Interesting. That #include compiles for me both with and without /permissive-.

Edit: no, my mistake, it fails with /permissive- with the error above.

jzmaddock commented 5 years ago

Edit: no, my mistake, it fails with /permissive- with the error above.

Got it. /permissive- is the default for new projects it seems so existing projects may not see this yet.

pdimov commented 5 years ago

OK, @ncook-vero, does https://github.com/boostorg/range/pull/70 work for you?

ncook-hxgn commented 5 years ago

Ok, here's what I've been trying - building a boost 1.67 containing one of these difference_type definitions, then building our project against it. Here's my sketchpad:

//#if defined(_MSC_VER) && _MSC_VER >= 1913
//      BOOST_DEDUCED_TYPENAME difference_type n;
//#else
//      BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n;
//      BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept<Iterator>::difference_type n;
            BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n;
//#endif

70 didn't work for me I'm afraid:

BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n; yields Error C4596 'difference_type': illegal qualified name in member declaration (compiling source file xxxxxxx.cpp) xxxxxxx d:\development\xxxxxxx\3rdparty\boost\boost\range\concepts.hpp 258

BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept<Iterator>::difference_type n; yields d:\development\xxxxxxx\3rdparty\boost\boost\range\concepts.hpp(258): error C4596: 'difference_type': illegal qualified name in member declaration (compiling source file xxxxxxx.cpp)

However, based on a previous suggestion, I tried BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n; and it worked. Given that this is essentially asking the parent class and template type to provide the definition (via ADL etc?), I think this essentially demonstrates the goodness of my original fix (I think it is essentially asking the compiler the same thing, isn't it?).

Am I missing something?

RE: /permissive-, we compile with /WAll, warnings-as-errors, etc. I saw /Zc:rvalueCast mentioned in the /permissive- MSDN page as turned on by /permissive- .. I also saw /Zc:rvalueCast in our project settings, so we are compiling with /permissive-

pdimov commented 5 years ago

OK, I'll do another PR with BidirectionalIterator instead.

pdimov commented 5 years ago

No success in reproducing the problem in a self-contained test?

ncook-hxgn commented 5 years ago

I'm attempting to create one now similar to @jzmaddock, just need to rebuild boost

ncook-hxgn commented 5 years ago

Reporting back on a repro outside of our product..

VS2017 15.7.4, Boost 1.67

In a harness (new Win32 console app), it is my change in concepts.hpp that causes the error C3646: 'n' : unknown override specifier - so I think I have reproduced what everyone else is seeing. The harness was compiled with /WAll /WX /permissive- /Yu in debug and /W4 /WX /permissive- /Yu in release (x64 only).

Worth noting that using BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n; in place of my change (i.e., the original code) fixes the error C3646: 'n' : unknown override specifier in the harness.

However, returning to our product which includes the very same header, with the same original-code-not-my-fix experimental change from the harness, the result is somewhat reversed and I get error C4596: 'difference_type': illegal qualified name in member declaration ...

Edit Edit:

The problem can be reproduced in a new VS2017 Win32 x64 project by disabling /permissive-. this doesn't happen including concepts.hpp on it's own, it seems it is something we are doing with the boost::geometry library - here's the offensive main.cpp

// TestBoostConcepts.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
// /WAll :
#pragma warning(disable: 4365)
#pragma warning(disable: 4371)
#pragma warning(disable: 4571)
#pragma warning(disable: 4514)
#pragma warning(disable: 4619)
#pragma warning(disable: 4625)
#pragma warning(disable: 4668)
#pragma warning(disable: 4626)
#pragma warning(disable: 4686)
#pragma warning(disable: 4710)
#pragma warning(disable: 4774)
#pragma warning(disable: 4820)
#pragma warning(disable: 5026)
#pragma warning(disable: 5027)

#pragma warning(disable: 4005)
#pragma warning(disable: 4100)
#pragma warning(disable: 5031)
#pragma warning(disable: 5032)

#include <boost/assign.hpp>
#include <boost/geometry/geometries/point_xy.hpp>
#include <boost/geometry/geometries/linestring.hpp> 
#include <boost/geometry/algorithms/intersects.hpp>
#include <boost/geometry/algorithms/append.hpp>

namespace boost
{
    namespace geometry
    {
        namespace dispatch
        {

            template <bool Reverse, typename LineString, typename TurnPolicy>
            struct self_get_turn_points<Reverse, linestring_tag, LineString, TurnPolicy>
                : detail::self_get_turn_points::get_turns<Reverse, TurnPolicy>
            {
            };
        }
    }
}

int main()
{
    using point_t = boost::geometry::model::d2::point_xy<double>;
    using linestring_t = boost::geometry::model::linestring<point_t>;

    using turn_info = boost::geometry::detail::overlay::turn_info<point_t, boost::geometry::segment_ratio<double>>;
    using rescale_policy_t = boost::geometry::detail::no_rescale_policy;
    using interrupt_policy_t = boost::geometry::detail::self_get_turn_points::no_interrupt_policy;

    using named_linestring = std::pair<size_t, linestring_t>;

    using strategy_t = typename boost::geometry::strategy::relate::services::default_strategy<std::list<named_linestring>::value_type::second_type, std::list<named_linestring>::value_type::second_type>::type;

    //
    std::list<named_linestring> inners;

    rescale_policy_t robust_policy;
    interrupt_policy_t policy;

    strategy_t strategy_inner;
    std::deque<turn_info> turns;
    for (const auto& inner_pair : inners)
    {
        const auto& inner = inner_pair.second;
        boost::geometry::self_turns<boost::geometry::detail::overlay::assign_null_policy>(inner, strategy_inner, robust_policy, turns, policy);
    }

    return 0;
}

Command line to fail: /Yu"stdafx.h" /GS- /GL /Wall /Gy /Zc:wchar_t /I"D:\Development\3rdParty\boost" /Zi /Gm- /O2 /Fd"x64\Release\vc141.pdb" /Zc:inline /fp:precise /D "NDEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /GF /WX /Zc:forScope /Gd /Oi /MD /FC /Fa"x64\Release\" /EHa /nologo /Fo"x64\Release\" /Fp"x64\Release\TestBoostConcepts.pch" /diagnostics:classic

Command line to succeed: /permissive- /Yu"stdafx.h" /GS- /GL /Wall /Gy /Zc:wchar_t /I"D:\Development\3rdParty\boost" /Zi /Gm- /O2 /Fd"x64\Release\vc141.pdb" /Zc:inline /fp:precise /D "NDEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /GF /WX /Zc:forScope /Gd /Oi /MD /FC /Fa"x64\Release\" /EHa /nologo /Fo"x64\Release\" /Fp"x64\Release\TestBoostConcepts.pch" /diagnostics:classic

Flamefire commented 5 years ago

Why is BOOST_DEDUCED_TYPENAME difference_type n; used and not simply difference_type n;?

vinniefalco commented 5 years ago

This is still a problem in 1.68 beta 1. Attempting to build all the boost libraries on my machine with /permissive- produces this output https://gist.github.com/vinniefalco/40b2e8e6a283377330258ce1e3281b6c

Flamefire commented 5 years ago

@vinniefalco Would you mind testing if removing the BOOST_DEDUCED_TYPENAME from the line at C:\Users\vinnie\src\boost\boost/range/concepts.hpp(257) (and only there) solves this for you?

vinniefalco commented 5 years ago

Removing BOOST_DEDUCED_TYPENAME still produces an error. However, changing it as follows resolves the error (I am on the latest VS)

#if defined(_MSC_VER) && _MSC_VER >= 1912 
    BOOST_DEDUCED_TYPENAME
    BidirectionalIteratorConcept<Iterator>::difference_type amount;
 #else
    BOOST_DEDUCED_TYPENAME
    RandomAccessIteratorConcept::difference_type amount;
 #endif
Flamefire commented 5 years ago

Does it produce the same error or a different one? And lastly: Would you mind testing BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type amount; too (or remove the first part of the #if)? If that works than previous discussion is right: The work-around should only be for a specific MSVC version or completely removed as the non-MSVC code is actually correct.

vinniefalco commented 5 years ago
vinniefalco commented 5 years ago

All the boost libraries build correctly with

BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n
Flamefire commented 5 years ago

Just seen that develop already contains the fixes: #70 and #71 This does not require macros. To summarize everything said and confirmed so far it is a bug in _MSC_VER==1912 only (original fix said >= 1912, but others mentioned 1913 is not affected and a more fine grained #if defined(_MSC_FULL_VER) && _MSC_FULL_VER > 191225830 && _MSC_FULL_VER < 191300000 was suggested)

Please get #70 and #71 merged into master for the 1.68 version and this can be closed.

ncook-hxgn commented 5 years ago

Okay (seems likely that I would stumble into a Visual Studio bug, that is my kind of luck), but what about our failed build? Simply removing /permissive- made it fail at that location - granted that the example code is mad..

Flamefire commented 5 years ago

Did you test with those 2 PRs? Or simply replace the full #if stuff here with BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n; and test that. Are there still errors in your build then? If so, could you post that error? Would be very interesting!

ncook-hxgn commented 5 years ago

I did, see above posts. I've tried all the permutations offered.

pdimov commented 5 years ago

You wrote above

However, based on a previous suggestion, I tried BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n; and it worked.

Is this not true?

ncook-hxgn commented 5 years ago

It is true. At least I think it is. I suggest trying my TestBoostConcepts.cpp sample in a VS 2017 15.7.4, toggling /permissive- in project settings, and trying the various difference_type definitions in concepts.hpp, that's what I did. /permissive- seemed to be key to the issue. Some of our projects use /permissive- and some don't. We need boost to work with all of them. The TestBoostConcepts.cpp code comes from a section of code in a project that is not compiled with /permissive-.

Flamefire commented 5 years ago

So where is the problem? If BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n; works for you then you did not test what I suggested in https://github.com/boostorg/range/issues/66#issuecomment-404427400

pdimov commented 5 years ago

Unfortunately, your sample compiles for me no matter which definition of n I use in concepts.hpp. My command line does not match exactly:

/GS /GL /W4 /Gy /Zc:wchar_t /I"/boost-git/develop" /Zi /Gm- /Ox /Ob2 /Fd"x64\Release\vc141.pdb" /Zc:inline /fp:precise /D "NDEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /WX- /Zc:forScope /Gd /Oi /MD /FC /Fa"x64\Release\" /EHsc /nologo /Fo"x64\Release\" /Fp"x64\Release\testbed2017.pch" /diagnostics:classic

but I don't see how any of those options would affect the front end. What do you have in stdafx.h?

pdimov commented 5 years ago

In either case, the develop branch now contains the BidirectionalIteratorConcept definition, so if you could check that your product compiles with it, we'd be done.

ncook-hxgn commented 5 years ago

stdafx.h contains:

// stdafx.h : include file for standard system include files,
// or project specific include files that are used frequently, but
// are changed infrequently
//

#pragma once

#define _CRT_SECURE_NO_WARNINGS
#define _SCL_SECURE_NO_WARNINGS

#include "targetver.h"

// TODO: reference additional headers your program requires here

I'll check again, bear with me. We are using /WAll /WX, so we must disable many warnings in the product, as in the sample. We disable others in the product, I just disabled what I had to so that the example would 'compile' at least as far as concepts.hpp.

ncook-hxgn commented 5 years ago

This built successfully:

         private:
#if defined(_MSC_VER) && _MSC_VER >= 1913
        BOOST_DEDUCED_TYPENAME BidirectionalIteratorConcept<Iterator>::difference_type n;
#else
        BOOST_DEDUCED_TYPENAME RandomAccessIteratorConcept::difference_type n;
#endif
pdimov commented 5 years ago

Why didn't you use the current contents of the develop branch?

https://github.com/boostorg/range/blob/70d1727ed301f2852ba911aacdcbcc57bb0ac92d/include/boost/range/concepts.hpp#L254-L257

ncook-hxgn commented 5 years ago

I'm #if defined(_MSC_VER) && _MSC_VER >= 1913

(Because it takes a long time to clone boost and I don't have time, tbh, never mind the whole FOSS version approval process and whatnot. Apologies).

Am I the only one who can close this? Closing.

Flamefire commented 5 years ago

Can this be simply closed please? The bug is obviously fixed/handled in develop and working for everyone so far.

pdimov commented 5 years ago

OK, I've taken the liberty of merging the fix to master.

Flamefire commented 5 years ago

Great! Make sure it gets included in the 1.68 release, please.

neilgroves commented 5 years ago

Peter,

Thank you enormously for your massive help in this. I was massively time constrained. Your help is really appreciated.

Thanks, Neil

On 12 July 2018 at 15:03, Peter Dimov notifications@github.com wrote:

OK, I've taken the liberty of merging the fix to master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/boostorg/range/issues/66#issuecomment-404523297, or mute the thread https://github.com/notifications/unsubscribe-auth/AGaE6m39SjBTDb7wxJkw_PAK0GjFpgShks5uF1c6gaJpZM4SX_Fa .

pabristow commented 5 years ago

As well as thanks to/from everyone, would it be useful to record why this concept fix, especially why bidirectional rather than randomaccess?

Paul

From: neilgroves [mailto:notifications@github.com] Sent: 12 July 2018 15:56 To: boostorg/range Cc: Paul A. Bristow; Comment Subject: Re: [boostorg/range] Error C4596: 'difference_type': illegal qualified name in member declaration when compiling range/concepts.hpp with MSVC141 (#66)

Peter,

Thank you enormously for your massive help in this. I was massively time constrained. Your help is really appreciated.

Thanks, Neil

On 12 July 2018 at 15:03, Peter Dimov notifications@github.com wrote:

OK, I've taken the liberty of merging the fix to master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/boostorg/range/issues/66#issuecomment-404523297, or mute the thread https://github.com/notifications/unsubscribe-auth/AGaE6m39SjBTDb7wxJkw_PAK0GjFpgShks5uF1c6gaJpZM4SX_Fa .

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/boostorg/range/issues/66#issuecomment-404541210, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABBuAdiMK9rkKb26UiaKFB4M3Gdsna6hks5uF2NzgaJpZM4SX_Fa.

Flamefire commented 5 years ago

As well as thanks to/from everyone, would it be useful to record why this concept fix, especially why bidirectional rather than randomaccess?

The type is not defined in the current type but in the parent type like here:

template<class T>
struct Foo{
  using type = T;
};
template<class T>
struct Bar: public Foo<T>{
  static typename Foo<T>::type value = T(0);
};

Not sure if static typename Bar::type value = T(0); is covered by the standard, but at least MSVC chocked on that (in at least 1 version) and I remember similar problems in other libraries too (using the parent type instead of the current fixed a problem)