clang++/libc++ support -- <iostream> must be included before it is used in hashtable-common.h

[GoogleCodeExporter]

hashtable-common.h defines several functions that operate on std::istream or 
std::ostream, without including <iostream>, with the following justification:

// Lucky these are inline, because we want the caller to be responsible
// for #including <iostream>, not us (iostream is a big header!)
template<typename Ignored>
inline bool read_data_internal(Ignored*, std::istream* fp,
                               void* data, size_t length) {
  return fp->read(reinterpret_cast<char*>(data), length).good();
}

Unfortunately, this does not seem to be standards-compliant (although g++ 
allows it) -- see http://clang.llvm.org/compatibility.html#undep_incomplete

Under clang++/libstdc++, this happens to work, because sparsehashtable.h and 
densehashtable.h both include <iterator> before including hashtable-common.h, 
and libstdc++'s <iterator> #include's <istream> and <ostream>.

libc++'s <iterator>, however, merely includes <iosfwd>, and so building 
sparsehash under clang++ and libc++ fails with

In file included from src/time_hash_map.cc:86:
In file included from ./src/google/dense_hash_map:104:
In file included from ./src/google/sparsehash/densehashtable.h:100:
./src/google/sparsehash/hashtable-common.h:102:12: error: implicit 
instantiation of undefined
      template 'std::__1::basic_istream<char, std::__1::char_traits<char> >'
  return fp->read(reinterpret_cast<char*>(data), length).good();
           ^
/usr/include/c++/v1/iosfwd:108:27: note: template is declared here
    class _LIBCPP_VISIBLE basic_istream;
                          ^

What steps will reproduce the problem?
1. Install clang++ and libc++, per instructions at http://libcxx.llvm.org/
2. Configure sparsehash with CXX='clang++ -stdlib=libc++'
3. Run make

What is the expected output? What do you see instead?

The build should succeed. Instead, it fails with the above error.

What version of the product are you using? On what operating system?

google-sparsehash r94
clang version 3.1 (trunk 145674)
libc++ r145624
Ubuntu 11.10 Oneiric

Please provide any additional information below.

Note that libc++ doesn't actually support enough exception-handling to *link* 
google-sparsehash (see http://libcxxabi.llvm.org/spec.html) even if the compile 
is fixed, but with this fix it at least builds, which is progress.

The attached patch fixes the issue, although it is not the only possible patch.

Original issue reported on code.google.com by nelh...@nelhage.com on 2 Dec 2011 at 11:24

Attachments:

• iostream.diff

[GoogleCodeExporter]

That's too bad; I was really hoping to avoid bringing in iostream here.  
iostream is a ginormous header file.

btw, thanks for the clang link; that explains things to me very clearly.  I 
have a loophole that I think should work, by making the stream a dependent 
type.  Can you try this patch on libc++ and see what happens?  (It may not 
apply cleanly due to other changes I have going on, but shouldn't be difficult 
to apply manually if need be.)

--- hashtable-common.h#16   2011-11-18 14:23:42.000000000 -0800
+++ hashtable-common.h  2011-12-02 16:05:10.580260000 -0800
@@ -100,18 +100,30 @@
   return fwrite(data, length, 1, fp) == 1;
 }

-// Lucky these are inline, because we want the caller to be responsible
-// for #including <iostream>, not us (iostream is a big header!)
+// We want the caller to be responsible for #including <iostream>, not
+// us, because iostream is a big header!  According to the standard,
+// it's only legal to delay the instantiation the way we want to if
+// the istream/ostream is a template type.  So we jump through hoops.
+template<typename ISTREAM>
+inline bool read_data_internal_for_istream(ISTREAM* fp,
+                                           void* data, size_t length) {
+  return fp->read(reinterpret_cast<char*>(data), length).good();
+}
 template<typename Ignored>
 inline bool read_data_internal(Ignored*, std::istream* fp,
                                void* data, size_t length) {
-  return fp->read(reinterpret_cast<char*>(data), length).good();
+  return read_data_internal_for_istream(fp, data, length);
 }

+template<typename OSTREAM>
+inline bool write_data_internal_for_ostream(OSTREAM* fp,
+                                            const void* data, size_t length) {
+  return fp->write(reinterpret_cast<const char*>(data), length).good();
+}
 template<typename Ignored>
 inline bool write_data_internal(Ignored*, std::ostream* fp,
                                 const void* data, size_t length) {
-  return fp->write(reinterpret_cast<const char*>(data), length).good();
+  return write_data_internal_for_ostream(fp, data, length);
 }

 // The INPUT type needs to support a Read() method that takes a

Original comment by csilv...@gmail.com on 3 Dec 2011 at 12:07

• Added labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

That patch works for me with clang++ with both libstdc++ and libc++.

As a point of interest, I decided to see how bad #include'ing <istream> and 
<iostream> was (A friend pointed out that <iostream> is particularly bad 
because it contains a static initializer -- static ios_base::Init __ioinit; -- 
but that we only need istream and ostream here).

I preprocessed time_hash_map.cc using both libstdc++ and libc++, with and 
without an #include of <istream> and <ostream>, and compared the number of 
lines in the resulting file:

libc++:
  37893 time_hash_map-time_hash_map.orig.libc++.E
  55896 time_hash_map-time_hash_map.ios.libc++.E

libstdc++:
  60657 time_hash_map-time_hash_map.orig.stdc++.E
  60657 time_hash_map-time_hash_map.ios.stdc++.E

So avoiding <istream> doesn't actually win *anything* on libstdc++, because 
it's already been pulled in by <iterator>. It's still a significant win on 
libc++, though.

Original comment by nelh...@nelhage.com on 3 Dec 2011 at 4:14

[GoogleCodeExporter]

Great (and thanks for the numbers!)  I'll get such a change into svn shortly.

Original comment by csilv...@gmail.com on 5 Dec 2011 at 5:39

• Changed state: Started

[GoogleCodeExporter]

OK, committed to SVN.

Original comment by csilv...@gmail.com on 7 Dec 2011 at 7:56

[GoogleCodeExporter]

This is now in sparsehash 1.12, just released.

Original comment by csilv...@gmail.com on 21 Dec 2011 at 5:27

• Changed state: Fixed