Skip to content

Turn off BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS for clang++ - #77

Merged
jeking3 merged 1 commit into
boostorg:developfrom
hia3:develop
Feb 19, 2019
Merged

Turn off BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS for clang++#77
jeking3 merged 1 commit into
boostorg:developfrom
hia3:develop

Conversation

@hia3

@hia3 hia3 commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@jeking3

jeking3 commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

We should probably switch to Boost.Predef here and detect dinkumware itself...

@hia3

hia3 commented Jan 14, 2019

Copy link
Copy Markdown
Contributor Author

I have two variants:

  1. Similar to this.

    #include <boost/predef.h>
    ...
    // I have no idea when Dinkumware added it, probably a lot
    // earlier than this check.
    #if defined(BOOST_LIB_STD_DINKUMWARE) && (BOOST_LIB_STD_DINKUMWARE >= BOOST_VERSION_NUMBER(6, 50, 0))

  2. Or:

    #include <boost/predef.h>
    ...
    #if defined(BOOST_LIB_STD_DINKUMWARE) && (_MSVC_STL_VERSION >= 141)

Trying to summon @BillyONeal

@BillyONeal

Copy link
Copy Markdown
Contributor

Hi there, my original PR didn't guard for compiler at all. I think this is a library property and _MSVC_STL_VERSION should be sufficient to identify but some maintainers were unhappy with that...

@BillyONeal

Copy link
Copy Markdown
Contributor

We should probably switch to Boost.Predef here and detect dinkumware itself...

The difficulty here is that MSVC++'s standard library is a Dinkumware derivative but is no longer actually Dinkumware; so using a Dinkumware test to detect us will likely do the wrong thing on other Dinkumware licensees (e.g. I've heard rumors that Playstation still uses Dinkumware?).

@jeking3

jeking3 commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

So _CPPLIB_VER identifies dinkumware, or a dinkumware derivitive, however we only want to set this BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS on MSVC if STL is version 140 or earlier, is that correct? And the change here is that MSVC started shipping with clang support built-in, so this change accounts for that case which was missing?

@BillyONeal

Copy link
Copy Markdown
Contributor

BOOST_IOSTREAMS_HAS_DINKUMWARE_FPOS on MSVC if STL is version 140 or earlier, is that correct?

Right; other Dinkumware licensees likely still have the bigger-than-2-GiB file handling bugs we fixed.

@pdimov

pdimov commented Jan 15, 2019

Copy link
Copy Markdown
Member

I see no reason to check the compiler macros here. _MSVC_STL_VERSION >= 141 should be enough.

edit: or rather, defined(_MSVC_STL_VERSION) && _MSVC_STL_VERSION >= 141 should be enough, to avoid a warning when _MSVC_STL_VERSION isn't defined.

@pdimov

pdimov commented Jan 15, 2019

Copy link
Copy Markdown
Member

Original PR for reference: #57

@jeking3

jeking3 commented Jan 15, 2019

Copy link
Copy Markdown
Contributor

That's what I was thinking, but I wanted to clarify. Thanks @pdimov!

@jeking3 jeking3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify this entire expression down to:

#if defined(_CPPLIB_VER) && (!defined(_MSVC_STL_VERSION) || _MSVC_STL_VERSION < 141)

perhaps?

@pdimov

pdimov commented Jan 15, 2019

Copy link
Copy Markdown
Member

_YVALS is probably obsolete; I don't think we support pre-_CPPLIB_VER Dinkumware. Checking __SGI_STL_PORT or _STLPORT_VERSION seems redundant; I don't see how you can use both Dinkumware and STLport at the same time. Although who knows what happens on esoteric platforms. __QNX__ and __VXWORKS__ are probably checked because they use Dinkumware too? Not sure.

@pdimov

pdimov commented Jan 15, 2019

Copy link
Copy Markdown
Member

#13 and #49 introduce the checks for VxWorks. Presumably QNX uses a similar Dinkumware version.

The STLPort checks come from 2d11830, without much of a history.

@pdimov

pdimov commented Jan 15, 2019

Copy link
Copy Markdown
Member

Some more history: 3a18bc4, ccf8548

@codecov

codecov Bot commented Jan 15, 2019

Copy link
Copy Markdown

Codecov Report

Merging #77 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #77   +/-   ##
========================================
  Coverage    64.73%   64.73%           
========================================
  Files           81       81           
  Lines         3547     3547           
  Branches       950      950           
========================================
  Hits          2296     2296           
  Misses         461      461           
  Partials       790      790

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29a8317...4d2bbc5. Read the comment docs.

@jeking3

jeking3 commented Jan 15, 2019

Copy link
Copy Markdown
Contributor

Sadly we cannot test these platforms in our CI environment, so the safest thing to do is leave what's there and make the smallest modification possible.

@jeking3
jeking3 merged commit 2543c0f into boostorg:develop Feb 19, 2019
@K-M-Ibrahim-Khalilullah

Copy link
Copy Markdown

@BillyONeal
I installed boost with pcl. Now how can I turn off for making it usable in my project

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

Successfully merging this pull request may close these issues.

5 participants