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

Replace filenames in reflection with filenames+includes. #6703

Merged
merged 5 commits into from
Jun 19, 2021

Conversation

CasperN
Copy link
Collaborator

@CasperN CasperN commented Jun 18, 2021

Adding the map of schema files to included schemas to reflection.
This feature was requested at the end of #6613.
I made a backwards incompatible change to fbs_files since it was added recently.
Also related: #6428.

@mikkelfj

This is needed for some use cases and may be just useful metadata.
@github-actions github-actions bot added c++ codegen Involving generating code from schema labels Jun 18, 2021
@mikkelfj
Copy link
Contributor

Can't speak to the implementation, but I am happy with the schema change in reflection.fbs.

I'm a bit concerned about the already implementated convetntion of having filenames being relative to the flatc invocation path by default. It could unintentionally leak information and is likely a relative path starting with '..' if invoked in a typically build environment where the build directory is separate from the source tree.

I'd suggest using the directory holding the root fbs file as the default relative path. (This assuming that multiple cli specified fbs files are not merged into a single bfbs file).

@CasperN
Copy link
Collaborator Author

CasperN commented Jun 19, 2021

convention of having filenames being relative to the flatc invocation

Its actually relative to a specified "project root", passed in by passing a --bfbs-filenames=$project_root flag. Defaulting to the relative to the first filename isn't a bad idea, although there's no guarantee that later filenames wouldn't have a "..".

@mikkelfj
Copy link
Contributor

Defaulting to the relative to the first filename isn't a bad idea, although there's no guarantee that later filenames wouldn't have a "..".

But that would be OK in that case.

@CasperN CasperN merged commit 06fd6d6 into google:master Jun 19, 2021
@CasperN CasperN deleted the includes branch August 16, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ codegen Involving generating code from schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants