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 unnecessary includes in code generated headers #4132

Merged
merged 9 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/re_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
//!
//! In order to extend the generated type declaration in the header,
//! you can specify a single code-block that you want to be injected into the type declaration by
//! starting it with `[CODEGEN COPY TO HEADER START]` and ending it with `[CODEGEN COPY TO HEADER END]`.
//! starting it with `<CODEGEN_COPY_TO_HEADER>` and ending it with `</CODEGEN_COPY_TO_HEADER>`.
//! Note that it is your responsibility to make sure that the cpp file is valid C++ code -
//! the code generator & build will not adjust the extension file for you!
//!
Expand Down
72 changes: 39 additions & 33 deletions crates/re_types_builder/src/codegen/cpp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,50 +207,56 @@ fn hpp_type_extensions(
return (quote! {}, None);
};

let target_header = format!("{filename_stem}.hpp");
for line in content.lines() {
if line.starts_with("#include") {
if let Some(start) = line.find('\"') {
let end = line.rfind('\"').unwrap_or_else(|| {
panic!("Expected to find '\"' include line {line} in file {extension_file:?}")
});
const COPY_TO_HEADER_START_MARKER: &str = "<CODEGEN_COPY_TO_HEADER>";
const COPY_TO_HEADER_END_MARKER: &str = "</CODEGEN_COPY_TO_HEADER>";

let include = &line[start + 1..end];
if include != target_header {
includes.insert_relative(include);
}
} else if let Some(start) = line.find('<') {
let end = line.rfind('>').unwrap_or_else(|| {
panic!(
let mut remaining_content = &content[..];
let mut hpp_extension_string = String::new();

while let Some(start) = remaining_content.find(COPY_TO_HEADER_START_MARKER) {
let end = remaining_content.find(COPY_TO_HEADER_END_MARKER).unwrap_or_else(||
panic!("C++ extension file has a start marker but no end marker. Expected to find '{COPY_TO_HEADER_END_MARKER}' in {extension_file:?}")
);
let end = remaining_content[..end].rfind('\n').unwrap_or_else(||
panic!("Expected line break at some point before {COPY_TO_HEADER_END_MARKER} in {extension_file:?}")
);

let extensions = &remaining_content[start + COPY_TO_HEADER_START_MARKER.len()..end];

// Comb through any includes in the extension string.
for line in extensions.lines() {
if line.starts_with("#include") {
if let Some(start) = line.find('\"') {
let end = line.rfind('\"').unwrap_or_else(|| {
panic!(
"Expected to find ending '\"' in include line {line} in file {extension_file:?}"
)
});

includes.insert_relative(&line[start + 1..end]);
} else if let Some(start) = line.find('<') {
let end = line.rfind('>').unwrap_or_else(|| {
panic!(
"Expected to find or '>' in include line {line} in file {extension_file:?}"
)
});
includes.insert_system(&line[start + 1..end]);
});
includes.insert_system(&line[start + 1..end]);
} else {
panic!("Expected to find '\"' or '<' in include line {line} in file {extension_file:?}");
}
} else {
panic!("Expected to find '\"' or '<' in include line {line} in file {extension_file:?}");
hpp_extension_string += line;
hpp_extension_string += "\n";
}
}
}

const COPY_TO_HEADER_START_MARKER: &str = "[CODEGEN COPY TO HEADER START]";
const COPY_TO_HEADER_END_MARKER: &str = "[CODEGEN COPY TO HEADER END]";

let start = content.find(COPY_TO_HEADER_START_MARKER).unwrap_or_else(||
panic!("C++ extension file missing start marker. Without it, nothing is exposed to the header, i.e. not accessible to the user. Expected to find '{COPY_TO_HEADER_START_MARKER}' in {extension_file:?}")
);

let end = content.find(COPY_TO_HEADER_END_MARKER).unwrap_or_else(||
panic!("C++ extension file has a start marker but no end marker. Expected to find '{COPY_TO_HEADER_START_MARKER}' in {extension_file:?}")
);
let end = content[..end].rfind('\n').unwrap_or_else(||
panic!("Expected line break at some point before {COPY_TO_HEADER_END_MARKER} in {extension_file:?}")
);
remaining_content = &remaining_content[end + COPY_TO_HEADER_END_MARKER.len()..];
}

let comment = quote_comment(&format!(
"Extensions to generated type defined in '{}'",
extension_file.file_name().unwrap()
));
let hpp_extension_string = content[start + COPY_TO_HEADER_START_MARKER.len()..end].to_owned();
let hpp_type_extensions = quote! {
public:
#NEWLINE_TOKEN
Expand Down Expand Up @@ -402,7 +408,7 @@ impl QuotedObject {
let method_ident = format_ident!("with_{}", obj_field.name);
let field_type = quote_archetype_field_type(&mut hpp_includes, obj_field);

hpp_includes.insert_rerun("util.hpp");
hpp_includes.insert_rerun("warning_macros.hpp");
let gcc_ignore_comment =
quote_comment("See: https://github.com/rerun-io/rerun/issues/4027");

Expand Down
1 change: 1 addition & 0 deletions docs/code-examples/asset3d_out_of_tree.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Log a simple 3D asset with an out-of-tree transform which will not affect its children.

#include <exception>
#include <filesystem>

#include <rerun.hpp>
#include <rerun/demo_utils.hpp>
Expand Down
2 changes: 1 addition & 1 deletion rerun_cpp/src/rerun/archetypes/arrows3d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rerun_cpp/src/rerun/archetypes/arrows3d_ext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

/// Creates new 3D arrows pointing in the given directions, with a base at the origin (0, 0,
/// 0).
Expand All @@ -16,7 +16,7 @@ namespace rerun {
return arrows;
}

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif
} // namespace archetypes
} // namespace rerun
5 changes: 1 addition & 4 deletions rerun_cpp/src/rerun/archetypes/asset3d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 11 additions & 6 deletions rerun_cpp/src/rerun/archetypes/asset3d_ext.cpp
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
#include <algorithm>
#include <filesystem> // TODO(#3991): Should not leak into public header.
#include <fstream> // TODO(#3991): Should not leak into public header.
#include <fstream>
#include <string>

#include "asset3d.hpp"

//#define EDIT_EXTENSION
// It's undefined behavior to pre-declare std types, see http://www.gotw.ca/gotw/034.htm
// We want to use `std::filesystem::path`, so we have it include it in the header.
// <CODEGEN_COPY_TO_HEADER>

#include <filesystem>

// </CODEGEN_COPY_TO_HEADER>

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.

// <CODEGEN_COPY_TO_HEADER>

static std::optional<rerun::components::MediaType> guess_media_type(
const std::filesystem::path& path
Expand All @@ -38,7 +43,7 @@ namespace rerun {
return asset;
}

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif

Result<Asset3D> Asset3D::from_file(const std::filesystem::path& path) {
Expand Down
1 change: 0 additions & 1 deletion rerun_cpp/src/rerun/archetypes/bar_chart.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rerun_cpp/src/rerun/archetypes/bar_chart_ext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

BarChart(rerun::datatypes::TensorBuffer buffer) {
auto num_elems = buffer.num_elems();
Expand Down Expand Up @@ -121,7 +121,7 @@ namespace rerun {
return BarChart(f64);
}

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif
} // namespace archetypes
} // namespace rerun
3 changes: 1 addition & 2 deletions rerun_cpp/src/rerun/archetypes/boxes2d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions rerun_cpp/src/rerun/archetypes/boxes2d_ext.cpp
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
#include "boxes2d.hpp"

#include "../component_batch_adapter_builtins.hpp" // TODO(#3991): Should not leak into public header.
#include "../component_batch_adapter_builtins.hpp"

// #define EDIT_EXTENSION

namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

/// Creates new `Boxes2D` with `half_sizes` centered around the local origin.
static Boxes2D from_half_sizes(ComponentBatch<components::HalfSizes2D> half_sizes) {
Expand Down Expand Up @@ -57,7 +57,7 @@ namespace rerun {
const std::vector<datatypes::Vec2D>& mins, const std::vector<datatypes::Vec2D>& sizes
);

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif
Boxes2D Boxes2D::from_sizes(const std::vector<datatypes::Vec2D>& sizes) {
std::vector<components::HalfSizes2D> half_sizes;
Expand Down
3 changes: 1 addition & 2 deletions rerun_cpp/src/rerun/archetypes/boxes3d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions rerun_cpp/src/rerun/archetypes/boxes3d_ext.cpp
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
#include "boxes3d.hpp"

#include "../component_batch_adapter_builtins.hpp" // TODO(#3991): Should not leak into public header.
#include "../component_batch_adapter_builtins.hpp"

// #define EDIT_EXTENSION

namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

/// Creates new `Boxes3D` with `half_sizes` centered around the local origin.
static Boxes3D from_half_sizes(ComponentBatch<components::HalfSizes3D> half_sizes) {
Expand Down Expand Up @@ -60,7 +60,7 @@ namespace rerun {
const std::vector<datatypes::Vec3D>& mins, const std::vector<datatypes::Vec3D>& sizes
);

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif
Boxes3D Boxes3D::from_sizes(const std::vector<datatypes::Vec3D>& sizes) {
std::vector<components::HalfSizes3D> half_sizes;
Expand Down
4 changes: 2 additions & 2 deletions rerun_cpp/src/rerun/archetypes/clear_ext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace rerun {
struct ClearExt {
rerun::components::ClearIsRecursive clear;

// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

static const Clear FLAT;

Expand All @@ -19,7 +19,7 @@ namespace rerun {
Clear(bool _is_recursive = false)
: Clear(components::ClearIsRecursive(_is_recursive)) {}

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
};
#endif

Expand Down
3 changes: 1 addition & 2 deletions rerun_cpp/src/rerun/archetypes/depth_image.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rerun_cpp/src/rerun/archetypes/depth_image_ext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

/// New depth image from height/width and tensor buffer.
///
Expand All @@ -23,7 +23,7 @@ namespace rerun {
/// Calls `Error::handle()` if the shape is not rank 2.
explicit DepthImage(components::TensorData _data);

// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif

DepthImage::DepthImage(components::TensorData _data) : data(std::move(_data)) {
Expand Down
3 changes: 1 addition & 2 deletions rerun_cpp/src/rerun/archetypes/image.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rerun_cpp/src/rerun/archetypes/image_ext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace rerun {
namespace archetypes {

#ifdef EDIT_EXTENSION
// [CODEGEN COPY TO HEADER START]
// <CODEGEN_COPY_TO_HEADER>

/// New Image from height/width/channel and tensor buffer.
///
Expand All @@ -22,7 +22,7 @@ namespace rerun {
/// Sets the dimension names to "height", "width" and "channel" if they are not specified.
/// Calls `Error::handle()` if the shape is not rank 2 or 3.
explicit Image(rerun::components::TensorData _data);
// [CODEGEN COPY TO HEADER END]
// </CODEGEN_COPY_TO_HEADER>
#endif

Image::Image(rerun::components::TensorData _data) : data(std::move(_data)) {
Expand Down
2 changes: 1 addition & 1 deletion rerun_cpp/src/rerun/archetypes/line_strips2d.hpp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading