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.

Sign up for GitHub

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

Jump to bottom

Fix unnecessary includes in code generated headers #4132

Merged
merged 9 commits into from
Nov 7, 2023
Merged

Fix unnecessary includes in code generated headers #4132

merged 9 commits into from
Nov 7, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 2, 2023 •
edited by github-actions bot
Loading

What

  • Fixes C++ don't leak headers from _ext impls into public headers #3991
  • Allow several _ext -> header copy blocks
  • no longer automatically add all includes from _ext file to header
  • detect includes in copy to header blocks and add them to the header list
  • rename util.hpp to warning_macros.hpp
  • change copy-to-codegen markers to an xml like syntax, shortening it a bit and making it easier to read

Oh and every _ext file I touched a bit more I cleaned up to use a less autocomplete friendly but easier to understand way of handling the extension code. Let's prefer this going forward (there's no functional difference though).

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf
Allow several _ext -> header copy blocks, no longer automatically add…
bd19d98 
… all includes from _ext file to header, detect includes in copy to header blocks, rename util.hpp to warning_macros.hpp
@Wumpf Wumpf added enhancement New feature or request 😤 annoying Something in the UI / SDK is annoying to use 🌊 C++ API C/C++ API specific include in changelog labels Nov 2, 2023
jleibs
jleibs approved these changes Nov 6, 2023
View reviewed changes
crates/re_types_builder/src/codegen/cpp/mod.rs Outdated Show resolved Hide resolved
crates/re_types_builder/src/codegen/cpp/mod.rs Outdated Show resolved Hide resolved
rerun_cpp/src/rerun/archetypes/asset3d_ext.cpp

namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
#if 0
Copy link
Member

Choose a reason for hiding this comment

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

Why not use EDIT_EXTENSION here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm phasing out the EDIT_EXTENSION way of doing things. The idea was to have LSP friendly code but that requires a bit of extra maintenance and everyone editing these (myself included) gets this wrong all the time. Also, it gave the impression that there's some additional magic at work.
So instead I make these being honest now and just explicitly disable stuff.

@Wumpf Wumpf merged commit 3279714 into main Nov 7, 2023
40 checks passed
@Wumpf Wumpf deleted the andreas/cpp/fix-ext-header-leaking branch November 7, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleibs jleibs jleibs approved these changes

Assignees
No one assigned
Labels
😤 annoying Something in the UI / SDK is annoying to use 🌊 C++ API C/C++ API specific enhancement New feature or request include in changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

C++ don't leak headers from _ext impls into public headers
2 participants
@Wumpf @jleibs