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

Cpp98 removal #1747

Merged
merged 18 commits into from
Feb 2, 2024
Merged

Cpp98 removal #1747

merged 18 commits into from
Feb 2, 2024

Conversation

pepone
Copy link
Member

@pepone pepone commented Feb 1, 2024

This PR removes the C++98 mapping and the usage of ICE_CPP11_MAPPING. We have a single C++ mapping now.

There are more things to cleanup, like removing the usage of helper macros like ICE_UNCHECKED_CAST, but I let this for follow-up PRs.

@pepone pepone marked this pull request as draft February 1, 2024 21:26
@pepone pepone marked this pull request as ready for review February 2, 2024 13:07
Copy link
Member

@bernardnormier bernardnormier left a comment

Choose a reason for hiding this comment

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

Looks good. There is only one CPP11 left:

cpp/doxygen/config-cpp11
2173:PREDEFINED             = ICE_CPP11_MAPPING

@@ -94,17 +99,17 @@
<ItemDefinitionGroup Condition="'$(ICE_BIN_DIST)' != 'all'">
<ClCompile>
<AdditionalIncludeDirectories>$(IceHome)\cpp\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories>$(IceHome)\cpp\include\generated\$(IceCppMapping)\$(Platform)\$(IceConfiguration);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories>$(IceHome)\cpp\include\generated\cpp11\$(Platform)\$(Configuration);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

Why cpp11?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's related to the TODO: remove cpp11 in ice.test.props.

@@ -987,95 +978,6 @@ Slice::Gen::generate(const UnitPtr& p)
Cpp11CompatibilityVisitor compatibilityVisitor(H, C, _dllExport);
p->visit(&compatibilityVisitor, false);
}

Copy link
Member

Choose a reason for hiding this comment

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

You should also remove all these C++98 visitors: ProxyDeclVisitor, DeclVisitor, etc. I am pretty sure they are independent of the Cpp11 visitors.

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

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

Looks good! We should wait for CI before merging.

Comment on lines 10 to 11
// Part of the C++98 mapping, and "internal" definitions when building Ice
// with the C++11 mapping
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove C++98 from the comment?

Copy link
Member

@externl externl Feb 2, 2024

Choose a reason for hiding this comment

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

Should this file be deleted? GitHub says it's just empty now. Though it might just be having issues with the size of this PR. :)

@pepone pepone merged commit 5491f90 into zeroc-ice:main Feb 2, 2024
17 checks passed
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.

3 participants