-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add support for nested namespaces for C++ #765
Add support for nested namespaces for C++ #765
Conversation
This CI failure seems spurious, like clang-14 isn't installed in the CI runner? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, small issue
I'll try to fix up CI in #768
tool/src/cpp/formatter.rs
Outdated
@@ -56,6 +56,7 @@ impl<'tcx> Cpp2Formatter<'tcx> { | |||
.rename | |||
.apply(resolved.name().as_str().into()); | |||
if let Some(ref ns) = resolved.attrs().namespace { | |||
let ns = ns.replace("::", std::path::MAIN_SEPARATOR_STR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Diplomat aims to generate the same code regardless of the host platform. AIUI forward slashes work on Windows in #include
, we should just support that, and we can add a config if someone wishes to use backslashes here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable
CI fixed |
I've updated with an improved function for computing relative paths to files which includes some minor isolated unit tests, and always uses '/' as the path separator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit worried about get_relative_path getting called too much, so it would be nice to have this be optimized with a more structured header path type, but this is fine for now. Small nit, can land.
tool/src/cpp/header.rs
Outdated
/// Returns the path to 'other' along with the # of steps back from 'root' required | ||
/// | ||
/// e.g get_nearest_root("/a/b/c/d.hpp", "a/b/z/d.hpp") -> "z/d.hpp", 2 | ||
/// or get_nearest_root("a/b.hpp", "root.hpp") -> "root.hpp", 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these docs are out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
e6ef3e4
to
6f7b963
Compare
Needs CI fixes |
6f7b963
to
098fee9
Compare
Adds support for using nested namespaces to the C/C++ generator. Nested namespaces are specified with the
::
separator syntax, ie "company::module::submodule". Filenames and include guards are updated so that multiple files with the same name in different namespaces don't conflict, which is a big part of the purpose of submodulesI'd add to the Book, but it seems the source isn't part of the repo?