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

Dynamic data loading should not use iostream #18

Closed
ghost opened this issue Jul 27, 2015 · 5 comments
Closed

Dynamic data loading should not use iostream #18

ghost opened this issue Jul 27, 2015 · 5 comments

Comments

@ghost
Copy link

ghost commented Jul 27, 2015

Originally reported on Google Code with ID 18

Dynamic data loading currently uses iostream for logging.

That would be fine, except that nowhere else in the library is iostream used, meaning
this is bringing in many classes for little gain, and only when dynamic data loading
is turned on.

Reported by csyoung@google.com on 2014-07-15 21:39:27

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Fair point. Do you have an alternative in mind? I can think of three:
1. Use printf instead. It's supposed to be much lighter weight.
2. Require a flag during build to create a "debug" version that include this error
logging
3. Both 1 and 2 above.

Thoughts?

Reported by andrewhayden@google.com on 2014-07-16 10:17:50

@ghost ghost self-assigned this Jul 27, 2015
@ghost
Copy link
Author

ghost commented Jul 27, 2015

I've thought a bit more about this and I think what we should do here is add a logging
helper to CLD2, and use that. We can then twiddle the logger on/off based on a DEFINE,
just like many other logging frameworks. I don't want to add a third-party logging
solution because the overhead just isn't worth it for our very sparse usage, at least
not at present.

We can switch to printf for now, and go down the define flag avenue later if it seems
worthwhile. Sound good?

Reported by andrewhayden@google.com on 2014-07-25 08:10:00

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Checked in compact_lang_det_impl.cc and found the standard pattern used here, which
is fprintf([stderr|stdout], ...). So that's the way to go.

Reported by andrewhayden@chromium.org on 2014-07-25 08:42:21

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Reported by andrewhayden@google.com on 2014-07-25 08:42:41

  • Status changed: Accepted

@ghost
Copy link
Author

ghost commented Jul 27, 2015

Committed as r164.

Reported by andrewhayden@google.com on 2014-07-25 11:48:23

  • Status changed: Fixed

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants