shaikat3 / open-vcdiff

Automatically exported from code.google.com/p/open-vcdiff
Apache License 2.0
0 stars 0 forks source link

open-vcdiff shouldn't log using cerr #33

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
#include <iostream> introduces static initializers, see http://crbug.com/94794

open-vcdiff logs through cerr. If open-vcdiff must log, it should use 
fprintf(stderr, ...) – or, probably better, provide a log handler that 
embedders could override to do what they please.

Original issue reported on code.google.com by thakis@google.com on 9 Dec 2011 at 3:58

GoogleCodeExporter commented 9 years ago

Original comment by thakis@google.com on 9 Dec 2011 at 3:58

GoogleCodeExporter commented 9 years ago
Lincoln: Can you comment on this?  

Thanks.

Original comment by j...@chromium.org on 9 Dec 2011 at 4:26

GoogleCodeExporter commented 9 years ago
This causes 12% of all files with static initializers in chrome/mac (not 
counting protobuf initializers). The good news is that all of them are just 
caused by the use of iostream.

Here's a list of the files with static initializers:
addrcache.o
codetable.o
decodetable.o
unicodetext.o
varint_bigendian.o
vcdecoder.o

Original comment by thakis@chromium.org on 9 Dec 2011 at 8:32

GoogleCodeExporter commented 9 years ago

Original comment by thakis@chromium.org on 9 Dec 2011 at 8:33

GoogleCodeExporter commented 9 years ago
thakis: the even-better news is that use of std::cerr doesn't have to trigger a 
static initializer, and that all the static initializers you found are caused 
by a single <iostream> include in logging.h (I think).   
http://codereview.appspot.com/5541062 reduces chrome's static initializer count 
from 116 to 109.

Original comment by fischman@chromium.org on 15 Jan 2012 at 8:20

GoogleCodeExporter commented 9 years ago
That's a start, but I guess the cc files that actually use these macros will 
need to include the definition as well, right?

Original comment by thakis@google.com on 15 Jan 2012 at 8:33

GoogleCodeExporter commented 9 years ago
#6: cc files don't need to include a definition.  class ostream is defined by 
#include <ostream> (note missing 'i') and extern declaration means only linker 
has to provide definition (which comes from the standard library).

Original comment by fischman@chromium.org on 15 Jan 2012 at 8:42

GoogleCodeExporter commented 9 years ago
Yeah, but I guess libstdc++ doesn't add that static initializer just for fun, 
there's probably some good reason for that :-)

On my system, /usr/include/c++/4.2.1/iostream looks fairly simple:

#ifndef _GLIBCXX_IOSTREAM
#define _GLIBCXX_IOSTREAM 1

#pragma GCC system_header

#include <bits/c++config.h>
#include <ostream>
#include <istream>

_GLIBCXX_BEGIN_NAMESPACE(std)

  /**
   *  @name Standard Stream Objects
   *
   *  The <iostream> header declares the eight <em>standard stream
   *  objects</em>.  For other declarations, see
   *  http://gcc.gnu.org/onlinedocs/libstdc++/27_io/howto.html#10 and the
   *  @link s27_2_iosfwd I/O forward declarations @endlink
   *
   *  They are required by default to cooperate with the global C library's
   *  @c FILE streams, and to be available during program startup and
   *  termination.  For more information, see the HOWTO linked to above.
  */
  //@{
  extern istream cin;       ///< Linked to standard input
  extern ostream cout;      ///< Linked to standard output
  extern ostream cerr;      ///< Linked to standard error (unbuffered)
  extern ostream clog;      ///< Linked to standard error (buffered)

#ifdef _GLIBCXX_USE_WCHAR_T
  extern wistream wcin;     ///< Linked to standard input
  extern wostream wcout;    ///< Linked to standard output
  extern wostream wcerr;    ///< Linked to standard error (unbuffered)
  extern wostream wclog;    ///< Linked to standard error (buffered)
#endif
  //@}

  // For construction of filebuffers for cout, cin, cerr, clog et. al.
  static ios_base::Init __ioinit;

_GLIBCXX_END_NAMESPACE

#endif /* _GLIBCXX_IOSTREAM */

It's exactly your declaration, and the static initializer. The comment above 
the initializer suggests fairly strongly that cerr relies on that static 
initializer having run before cerr is used.

Original comment by thakis@google.com on 15 Jan 2012 at 8:49

GoogleCodeExporter commented 9 years ago
Oops, you're right (27.4.2.1.6 in C++98).  
Sorry for the noise.

Original comment by fischman@chromium.org on 15 Jan 2012 at 9:08

GoogleCodeExporter commented 9 years ago

Original comment by yoz@chromium.org on 8 Aug 2012 at 11:59

GoogleCodeExporter commented 9 years ago
We managed to work around this in chromium without changes in open-vcdiff ( 
https://codereview.chromium.org/68253006 ).

Original comment by thakis@chromium.org on 13 Nov 2013 at 5:18

GoogleCodeExporter commented 9 years ago

Original comment by thakis@chromium.org on 13 Nov 2013 at 5:19