-
Notifications
You must be signed in to change notification settings - Fork 22
feat: add name mapping #93
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
Conversation
namespace { | ||
|
||
// Helper function to join a list of field names with a dot | ||
std::string JoinByDot(std::span<const std::string> parts) { |
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.
I guess you can use join_view
for this too
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.
I tried it but it seems that Clang does not yet implement this.
@@ -0,0 +1,61 @@ | |||
/* |
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.
Should we install this header by removing the internal suffix? I don't see it being used in this PR, but in #94, it's included in visit_type.h, which is meant to be installed for type manipulation. If we don't install this header, it might lead to compilation issues.
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.
I was using it for the visitor but not it is not required. Let me remove it for now. I saw that you have added that in your PR.
@@ -36,7 +37,6 @@ set(ICEBERG_SOURCES | |||
transform.cc | |||
transform_function.cc | |||
type.cc | |||
snapshot.cc |
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.
Not a CPP guy, but why is snapshot.cc
removed here? 🤔
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.
It is duplicate at line 32 :)
No description provided.