pybind visbility werrors#1054
Conversation
📝 WalkthroughWalkthroughVisibility attributes were added to several class and struct declarations to restrict symbol export in shared libraries, using Changes
Sequence Diagram(s)No sequence diagram generated as the changes are limited to symbol visibility, type qualification, and include reordering without control flow or feature modifications. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/python3/particles.hpp (1)
9-11: Consider conventional include ordering.The standard library includes (
<tuple>,<cassert>,<cstddef>) are now placed after the project-specificpybind_def.hppinclude. Conventional C++ practice typically places standard library headers first, followed by third-party headers, then project-specific headers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/python3/data_wrangler.hpp(2 hunks)src/python3/particles.hpp(1 hunks)src/python3/patch_data.hpp(1 hunks)src/python3/patch_level.hpp(2 hunks)src/python3/pybind_def.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
src/python3/patch_data.hpp (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
🔇 Additional comments (7)
src/python3/patch_data.hpp (2)
4-5: Good header organization.Moving
pybind_def.hppto the top aligns well with its usage in the visibility-controlled struct below.
14-14: Excellent symbol visibility control.The hidden visibility attribute correctly restricts symbol exports for the
PatchDatastruct, which is appropriate for internal Python binding infrastructure.src/python3/data_wrangler.hpp (2)
61-61: Correct symbol visibility control.The hidden visibility attribute properly restricts symbol exports for the
DataWranglerclass, which is appropriate for internal Python binding infrastructure.
91-91: Improved const qualifier placement.The const qualifier placement in
PatchData<std::vector<double>, dimension> const*is more consistent with modern C++ style than the previousconst PatchData<...>*form, while maintaining identical semantics.src/python3/pybind_def.hpp (1)
39-39: Proper symbol visibility control.The hidden visibility attribute correctly restricts symbol exports for the
PyArrayWrapperclass template, which is appropriate for internal Python binding infrastructure.src/python3/patch_level.hpp (2)
15-15: Correct symbol visibility control.The hidden visibility attribute properly restricts symbol exports for the
PatchLevelclass template, which is appropriate for internal Python binding infrastructure.
62-63: Clean formatting improvement.The function call formatting enhances readability by properly aligning the arguments across lines.
2 participants
Seeing a werror with a newer compiler for not respecting the visibility settings of pybind classes