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

<typeinfo> unconditionally included on dinkumware #306

Open
Kojoley opened this issue Dec 2, 2019 · 12 comments · May be fixed by #307
Open

<typeinfo> unconditionally included on dinkumware #306

Kojoley opened this issue Dec 2, 2019 · 12 comments · May be fixed by #307

Comments

@Kojoley
Copy link
Contributor

Kojoley commented Dec 2, 2019

It was added in 668b3fc to fix lexical_cast (https://svn.boost.org/trac10/ticket/4115) problem, but that seems to be an overkill (and probably not needed since boostorg/lexical_cast@3ce36a2?).
Also, it is a strange thing that with defined BOOST_NO_STD_TYPEINFO there is std::typeinfo. It completely undermines the purpose of the macro and of boost/core/typeinfo.hpp.

@Kojoley Kojoley linked a pull request Dec 2, 2019 that will close this issue
@Kojoley
Copy link
Contributor Author

Kojoley commented Dec 2, 2019

I have reproduced the https://svn.boost.org/trac10/ticket/4115 problem by applying boostorg/lexical_cast#31 and #307 on VC9.0/10.0/11.0, the fix for LexicalCast proposed in boostorg/lexical_cast#32.

@jzmaddock
Copy link
Collaborator

I'm curious to know why you think injecting typeinfo into the namespace in which it really should be defined anyway is such a bad idea?
Though we should certainly restrict the workaround to pre-msvc-12, as it's only required for obsolete compilers anyway.

@Kojoley
Copy link
Contributor Author

Kojoley commented Dec 3, 2019

I did not say it is a bad idea, it is convenient but costly.

@jeremy-murphy
Copy link

jeremy-murphy commented Mar 22, 2020

The IAR compiler for Arm uses Dinkumware and if I disable exceptions, which is admittedly I guess a non-standard extension, then it fails to compile this injection because it does not have ::typeinfo.
So, I would be in favour of limiting this kind of thing to only the compilers where it is known to work, rather than assuming it works in general.

@jeremy-murphy
Copy link

Digging around more on this, I'm not sure that #307 is the right solution after all.

I fixed this another way by a very simple modification to dinkumware.hpp on line 89, which currently is:

#if BOOST_MSVC < 1800

but I think it should be:

#if defined(_MSC_VER) && BOOST_MSVC < 1800

What do you think, @jzmaddock? I believe you added that line 6 months ago. :)

@jzmaddock
Copy link
Collaborator

You're correct: that's supposed to detect old MSVC versions, checking for defined(BOOST_MSVC) should do the trick also?

@Kojoley
Copy link
Contributor Author

Kojoley commented Jul 8, 2020

#307 is the right way, because Boost.TypeInfo is a portable solution to the problem the code is solving.

@Kojoley
Copy link
Contributor Author

Kojoley commented Jul 8, 2020

You're correct: that's supposed to detect old MSVC versions, checking for defined(BOOST_MSVC) should do the trick also?

defined(__ghs__) is clearly detecting something different than MSVC

@glenfe
Copy link
Member

glenfe commented Jul 8, 2020

You're correct: that's supposed to detect old MSVC versions, checking for defined(BOOST_MSVC) should do the trick also?

Yes to defined(BOOST_MSVC) since the whole point of BOOST_MSVC is avoid checking _MSC_VER which MSVC-pretenders define.

@jeremy-murphy
Copy link

You're correct: that's supposed to detect old MSVC versions, checking for defined(BOOST_MSVC) should do the trick also?

Sure, I was just suggesting what I already saw used repeatedly within that file, but now I realize that they were testing the version of _MSC_VER whereas this is BOOST_MSVC.
I trust you all know these macros better than I do.

@jeremy-murphy
Copy link

Even though we might agree about that line 89 needing a defined(BOOST_MSVC), it is actually tangential to this issue, which is about unconditionally including <typeinfo>, when there is an existing cross-library solution for it in Boost.Core.

I'm inclined to agree with @Kojoley that their solution is the right way to go, but I think the PR needs to also remove all the references to BOOST_NO_STD_TYPEINFO from the tests, etc? Since I think Dinkumware is the only place that defines it.

So, I would be super happy about someone putting the defined(BOOST_MSVC) in line 89 in the interim, to at least fix the broken conditional.

@GregKon
Copy link

GregKon commented Dec 13, 2020

Even though we might agree about that line 89 needing a defined(BOOST_MSVC), it is actually tangential to this issue, which is about unconditionally including <typeinfo>, when there is an existing cross-library solution for it in Boost.Core.

I'm inclined to agree with @Kojoley that their solution is the right way to go, but I think the PR needs to also remove all the references to BOOST_NO_STD_TYPEINFO from the tests, etc? Since I think Dinkumware is the only place that defines it.

So, I would be super happy about someone putting the defined(BOOST_MSVC) in line 89 in the interim, to at least fix the broken conditional.

I does confirm problem (IAR) and possibly solution works.

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 a pull request may close this issue.

5 participants