search

redserpent7 / cld2

Automatically exported from code.google.com/p/cld2
0 stars 0 forks source link

Dynamic data loading should not use iostream #18

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
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.

Original issue reported on code.google.com by csyo...@google.com on 15 Jul 2014 at 9:39

GoogleCodeExporter commented 9 years ago 
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?

Original comment by andrewha...@google.com on 16 Jul 2014 at 10:17

GoogleCodeExporter commented 9 years ago 
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?

Original comment by andrewha...@google.com on 25 Jul 2014 at 8:10

GoogleCodeExporter commented 9 years ago 
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.

Original comment by andrewha...@chromium.org on 25 Jul 2014 at 8:42

GoogleCodeExporter commented 9 years ago

Original comment by andrewha...@google.com on 25 Jul 2014 at 8:42

GoogleCodeExporter commented 9 years ago 
Committed as r164.

Original comment by andrewha...@google.com on 25 Jul 2014 at 11:48