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

Fix -Wsubobject-linkage warning #6243

Merged
merged 3 commits into from
Aug 4, 2024

Conversation

buzzhuzz
Copy link
Contributor

@buzzhuzz buzzhuzz commented Jul 24, 2024

Description

[8/476 0.2/sec] Building CXX object src/CMakeFiles/OrcaSlicer_profile_validator.dir/OrcaSlicer_profile_validator.cpp.o
In file included from /opt/packages/3d/OrcaSlicer/src/libslic3r/GCode.hpp:26,
                 from /opt/packages/3d/OrcaSlicer/src/libslic3r/calib.hpp:5,
                 from /opt/packages/3d/OrcaSlicer/src/libslic3r/AppConfig.hpp:12,
                 from /opt/packages/3d/OrcaSlicer/src/libslic3r/PresetBundle.hpp:5,
                 from /opt/packages/3d/OrcaSlicer/src/OrcaSlicer_profile_validator.cpp:3:
/opt/packages/3d/OrcaSlicer/src/libslic3r/GCode/SmallAreaInfillFlowCompensator.hpp:12:7: warning: ‘Slic3r::SmallAreaInfillFlowCompensator’ has a field ‘std::unique_ptr<{anonymous}::tk::spline> Slic3r::SmallAreaInfillFlowCompensator::flowModel’ whose type has internal linkage [-Wsubobject-linkage]
   12 | class SmallAreaInfillFlowCompensator
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Having tk::spline header-only implementation included from SmallAreaInfillFlowCompensator.hpp makes
SmallAreaInfillFlowCompensator::flowModel have separate (albeit the same) implementation in each translation unit.

In order to fix this issue, SmallAreaInfillFlowCompensator::flowModel converted to opaque 'pimpl'

Tests

Fixes ~240 compilation warnings

Having tk::spline header-only implementation included from
SmallAreaInfillFlowCompensator.hpp makes
SmallAreaInfillFlowCompensator::flowModel have separate (albeit the
same) implementation in each translation unit.

In order to fix this issue, SmallAreaInfillFlowCompensator::flowModel
converted to opaque 'pimpl'
@buzzhuzz
Copy link
Contributor Author

@mjonuschat , could you please help review this change?

@buzzhuzz
Copy link
Contributor Author

Just found that PR #5963 adds warning suppression using pragmas. Unfortunately, this leads to -Wunknown-warning warnigs on clang builds:

/opt/3d/OrcaSlicer/src/libslic3r/GCode/SmallAreaInfillFlowCompensator.hpp:17: warning: unknown warning group '-Wsubobject-linkage', ignored [-Wunknown-warning-option]
In file included from /opt/3d/OrcaSlicer/src/libslic3r/GCode/SmallAreaInfillFlowCompensator.cpp:17:
/opt/3d/OrcaSlicer/src/libslic3r/GCode/SmallAreaInfillFlowCompensator.hpp:17:32: warning: unknown warning group '-Wsubobject-linkage', ignored [-Wunknown-warning-option]
   17 | #pragma GCC diagnostic ignored "-Wsubobject-linkage"
      |                                ^

Which adds extra points to this PR

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good.

I'm not very clear about the purpose of TK_SPLINE_OUTER_NAMESPACE macro, could you explain it?

@buzzhuzz
Copy link
Contributor Author

buzzhuzz commented Aug 3, 2024

Types within anonymous namespace cannot be forward declared.

I've added a macro to keep an option to include spline.h within anonymous workspace if needed.

@SoftFever
Copy link
Owner

Would it be simpler if we move the spline out of the anonymous namespace?
It's still in tk namespace, so I think it won't cause any issue

@buzzhuzz
Copy link
Contributor Author

buzzhuzz commented Aug 3, 2024

That would also work.

Remove outer anonymous namespace from splice.h to make
forward declaration for tk::spline possible.
Copy link
Owner

@SoftFever SoftFever 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
Thank you

@SoftFever SoftFever merged commit e6ed93f into SoftFever:main Aug 4, 2024
15 checks passed
@buzzhuzz buzzhuzz deleted the dbuzz/wsubobject-linkage branch August 4, 2024 03:05
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.

2 participants