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 #include <iostream> with #include <ostream> #5

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Replace #include <iostream> with #include <ostream> #5

merged 1 commit into from
Nov 20, 2020

Conversation

brooksprumo
Copy link
Contributor

Including iostream also pulls in locale and other static data that adds
significant size to the compiled binary.

Since tser only uses ostream, replace #include <iostream> with
#include <ostream> to make tser usable in applications that are
sensitive to binary size.

@brooksprumo
Copy link
Contributor Author

brooksprumo commented Nov 19, 2020

"Significant size" applies to my use case, which is embedded firmware. We have ~512 KB of Flash for the whole binary, and including iostream adds ~220 KB by itself, which is a show-stopper.

Basically, any library that includes iostream is unusable for my platform (and likely most embedded platforms).

@KonanM
Copy link
Owner

KonanM commented Nov 19, 2020

Thanks for the feedback. Wouldn't it then even make more sense to include instead of ? Another option would be to template the ostream, but I guess that might be overkill?

For my main usecase I always needed std::cout so I included iostream directly, but it would not be a big hassle to change to iosfwd. That being said I would have to adapt some examples and the godbolt links so that everything still compiles and the test suite is ok.

@brooksprumo
Copy link
Contributor Author

Another option would be to template the ostream, but I guess that might be overkill?

ostream isn't a problem. You could template on the output stream, but you don't need to.

For my main usecase I always needed std::cout so I included iostream directly, but it would not be a big hassle to change to iosfwd. That being said I would have to adapt some examples and the godbolt links so that everything still compiles and the test suite is ok.

This makes sense. I know for me, adding another include to my application for iostream wouldn't be a problem (for my non-embedded applications).

I can also update the examples/godbolt links as well. Let me know what works for you!

Also, thanks for making this library!

@KonanM
Copy link
Owner

KonanM commented Nov 19, 2020

Alright I will accept your PR if you fix the examples and godbolt links. I actually wanted to say (iny my comment above) that we could also just include iosfwd instead of ostream?

Including iostream also pulls in locale and other static data that adds
significant size to the compiled binary.

Since tser only uses ostream, replace `#include <iostream>` with
`#include <ostream>` to make tser usable in applications that are
sensitive to binary size.

Another option would be to `#include <iosfwd>` instead, but that would
require all clients to then `#include <ostream>` themselves.

This commit also updates the examples, and the Compiler Explorer
links/examples in the documentation.
@brooksprumo
Copy link
Contributor Author

brooksprumo commented Nov 20, 2020

OK! I've updated the Compiler Explorer examples, updated their links in the README, updated the README code, and updated the non-single header version.

While we could #include <iosfwd> instead, but that would require all clients to then #include <ostream> themselves. I don't see that as a good user experience, personally.

For the examples, I think it makes sense for the main.cpp to #include <iostream> since those cpp files are calling std::cout explicitly, which goes along with Include What You Use.

Let me know how this looks!

@KonanM KonanM merged commit 672b64a into KonanM:master Nov 20, 2020
@KonanM
Copy link
Owner

KonanM commented Nov 20, 2020

Looks good to me now! Thanks for your contribution.

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.

2 participants