Implement --file-names-only
#8788
Conversation
github-actions
bot
added
c#
c++
codegen
|
| } | ||
|
|
||
| private: | ||
| bool SaveStub(const std::string& filename, const Imports& imports, |
There was a problem hiding this comment.
Same here, if making these private is needed for the functionality, sure, but if its a drive-by "I am making the code better" you better be damn sure its near impossible nobody could be calling these.
There was a problem hiding this comment.
agreed - in this case I think this is safe -- these were just free functions present in this cc file. I've just moved them to a more convenient location. Technically speaking these may have been accessible as they had external linkage... but anyone accessing these particular functions seems like a stretch.
include/flatbuffers/idl.h
Outdated
| struct IDLOptions { | ||
| // file saver | ||
| // shared pointer since this object gets copied and modified. | ||
| std::shared_ptr<FileSaver> file_saver = std::make_shared<RealFileSaver>(); |
There was a problem hiding this comment.
I am definitely on team "shared_ptr is a code smell", and I don't think we use them anywhere else in FlatBuffers unless they have crept in. If you made this a unique_ptr, what exactly would not work and why would that result in worse code? This is a new variable and I only see very basic references to it in this PR.
There was a problem hiding this comment.
100% agree -- I forget the exact thinking I had here but I believe it was because unique_ptr makes IDLOptions non-copyable, and I need someone to own the file saver. I can look into if idloptions can be safely moved around, or have idloptions just hold a raw pointer and have flatc own the saver (the latter is probably best)
There was a problem hiding this comment.
managed to refactor this to use an external unique_ptr and just have the options hold onto a raw pointer. IDLOptions is copied around way too many places to refactor, and we do need a single resource for the file name collection.
jtdavis777
left a comment
jtdavis777
left a comment
There was a problem hiding this comment.
@aardappel thanks for the thorough review! I can go back and not be so heavy handed with my cleanup.
Most of the file manager code that I cleaned up was brought in #7821, which was never used in our repo. I respect the callout that other people ingesting our code base may take advantage of them though.
include/flatbuffers/idl.h
Outdated
| struct IDLOptions { | ||
| // file saver | ||
| // shared pointer since this object gets copied and modified. | ||
| std::shared_ptr<FileSaver> file_saver = std::make_shared<RealFileSaver>(); |
There was a problem hiding this comment.
100% agree -- I forget the exact thinking I had here but I believe it was because unique_ptr makes IDLOptions non-copyable, and I need someone to own the file saver. I can look into if idloptions can be safely moved around, or have idloptions just hold a raw pointer and have flatc own the saver (the latter is probably best)
|
Ok, I'd agree any code that comes from an incomplete/non-functional PR that was merged is probably good to delete if not used in your fix. Still would prefer to minimize renames. |
|
seems like a decent middle ground. I'll compare with the original and roll back to being less aggressive, and will put back the helper functions. |
2 participants
Fixes #8784
Adds (my best attempt at) a minimally invasive implementation of
--file-names-only.Note that this version prints every file the program attempts to write -- during testing I found #8787 where python tries and silently fails to make a file. Other languages may have the same problem.