Skip to content

Conversation

@alexmillane
Copy link
Collaborator

@alexmillane alexmillane commented Jul 28, 2020

I needed to read protobuf messages from a stream of data in memory (as opposed to in a file). I swapped loading function inputs from fstream* to istream*. They were being implicitly cast to those types anyway, when passed to protobuf functions.

I also corrected a naming typo... This is a breaking change for anyone depending on this function... But it was quite confusing.

on top of #332

namespace utils {
bool readProtoMsgCountToStream(std::fstream* stream_in, uint32_t* message_count,
uint64_t* byte_offset);
bool readProtoMsgCountFromStream(std::istream* stream_in,
Copy link
Collaborator Author

@alexmillane alexmillane Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a naming correction that will break depending packages if they've implemented their own loading functions.

Obviously this function is reading from a stream, rather than to a stream.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to rename this. AFAIK this should only be used deep in the internals of this stuff (note to self for future designs: hide functions like these :) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People implementing new layers, submap types etc have to call this function, which does seem a bit odd to me, as we've discussed before :). Imma merge and let's see if anyone complains.

Copy link
Collaborator

@helenol helenol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

namespace utils {
bool readProtoMsgCountToStream(std::fstream* stream_in, uint32_t* message_count,
uint64_t* byte_offset);
bool readProtoMsgCountFromStream(std::istream* stream_in,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to rename this. AFAIK this should only be used deep in the internals of this stuff (note to self for future designs: hide functions like these :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants