Util/ClusterReader.cpp implicit type conversion an issue with compilers with C++11 as default language standard

[SHuang-Broad]

Hi, there seem to be an issue on line 70 when compiled with C++14 (either clang or gcc),

bool good = getline(*m_pReader, line);

which reports

error: no viable conversion from 'basic_istream<char, std::__1::char_traits<char> >' to 'bool'

turning off the C++14 support seem to solve the problem. Or a simpler solution would be to replace the line with minimal change:

bool good = getline(*m_pReader, line).good();

[mateidavid]

The program is written using the pre-C++11, so really C++03, language standard, and this particular issue states that it doesn't compile with C++14. Can you explain why would you expect it should? These are occasionally backward incompatible standards (see FTR below). To emphasize this point, would you consider it a valid issue to say it doesn't compile as C#? What would be the benefit of actually porting it to a newer standard (C++11 or C++14), as opposed to simply instructing the compiler to use the older standard, which it normally knows about?

There might well be a valid issue here, which is that the configuration flags do not explicitly ask for -std=c++03. The reason is that at the time this was written, C++03 was the default. Perhaps the cause for #106 is that gcc-6.0 changed to a newer default language standard. We don't have easy access to gcc-6.0 in order to check. But specifically asking for C++14 (and observing it doesn't work), is not necessarily a valid issue.

FTR: The backward incompatibility here is that pre-C++11, std::basic_ios is implicitly convertible to bool via void*; while after C++11, the conversion to bool is direct but explicit: http://en.cppreference.com/w/cpp/io/basic_ios/operator_bool .

[SHuang-Broad]

I agree, and have changed the title of the issue accordingly.

What I meant is that when users compile SGA, the compilers will very likely issue errors on this in the future.

It would be nice to have what you suggested: to change the configuration flag to explicitly ask for C++03, now that C++ is (and compilers are as well) evolving rapidly.

But IMO, the fix I suggested (or any explicit type conversion) is not that a big change, at least not asking for a C# port.

[mateidavid]

I agree that particular change is small, I think it's in only 2 places. But there's always the chance something else will pop up, for instance some other type conversion which doesn't work exactly as before. As long as it's easier to instruct the compiler to use the older standard, I see little motivation to do anything else.

So, which compiler versions did you encounter this issue on? Here we're using gcc-4.9.[12] on trusty. I tried 5.2.1 in a willy docker image, and again it worked just fine. The other issue mentioned 6.0 - is this what you used as well? Do you know where I can find a docker image with 6.0? I compiled gcc several times before and it's never pleasant.

Alternatively, can you try -std=c++03 on your end? An easy way would be: ./autogen [...], ./configure [...], then make CXXFLAGS="-O3 -std=c++03", then maybe make install. If it works, I'll just add it to https://github.com/jts/sga/blob/master/src/configure.ac#L94.

[SHuang-Broad]

Hi @mateidavid, admittedly, I didn't compile the SGA program but rather some other program that incorporates SGA, and specifically asks for C++11 as the language standard. But since SGA provides an opportunity for being imported, the use case is valid for SGA to consider. And the user community would appreciate it.

I've confirmed (while, not confirmed, but found an indication strong enough) by running the following test program, that the language standard is the cause:

#include <fstream>
#include <string>

int main(int argc, const char* argv[])
{
    std::string s;
    std::ifstream file_stream(argv[1]);
    bool good = std::getline(file_stream, s);
}

Compiling with several options, both clang and gcc, when asked specifically for C++11/14, issues the error.

Thank you.
Hope this helps.

clang_APPLE.7.0.2_verbose.txt
gcc_5.3.0_verbose.txt
clang_APPLE.7.0.2_C++11_verbose.txt
clang_APPLE.7.0.2_C++14_verbose.txt
gcc_5.3.0_C++11_verbose.txt
gcc_5.3.0_C++14_verbose.txt

[mateidavid]

We should leave this open for Jared to have a look.

I agree that integration of SGA into other projects (or import of some other project into SGA, if necessary) is a more compelling reason to make it C++11 friendly than the fact that compilers occasionally change their default dialect. Is the enclosing project using SGA build products (executables) or SGA source files? If it is using SGA build products, the enclosing project should simply take care not to pass flags such as -std=c++11 to the SGA build scripts. If it is using source files, then indeed there might be an issue here, different than the one about the compiler default C++ dialect.

[SHuang-Broad]

Sorry, pressed the wrong button and accidentally closed. Now reopened.

The tool (not finished product yet I believe) imports SGA as source, so yes, this is a valid reason for the team to make SGA more future-proof, as I believe there will be more developers doing this since SGA is a highly-regarded tool.

[dodomorandi]

I just upgraded to the new GCC 6.1, which uses -std=c++14 as default. I recompiled SGA (actually, I recompiled the SnowTools, which are based on SGA) and I got a couple of minor issues. Give a look to this simple patch:

diff --git a/SGA/Bigraph/Bigraph.h b/SGA/Bigraph/Bigraph.h
index 7da5b64..e9ad0fa 100644
--- a/SGA/Bigraph/Bigraph.h
+++ b/SGA/Bigraph/Bigraph.h
@@ -23,7 +23,7 @@
 //
 //typedef std::map<VertexID, Vertex*> VertexPtrMap;
 //typedef __gnu_cxx::hash_map<VertexID, Vertex*> VertexPtrMap;
-typedef std::tr1::unordered_map<VertexID, Vertex*> VertexPtrMap;
+typedef std::unordered_map<VertexID, Vertex*> VertexPtrMap;
 //typedef SparseHashMap<VertexID, Vertex*, StringHasher> VertexPtrMap;// JEREMIAH

 typedef VertexPtrMap::iterator VertexPtrMapIter;
diff --git a/SGA/SuffixTools/STCommon.h b/SGA/SuffixTools/STCommon.h
index 13e0769..b43477d 100644
--- a/SGA/SuffixTools/STCommon.h
+++ b/SGA/SuffixTools/STCommon.h
@@ -96,7 +96,7 @@ struct SAElem
         // Masks
         static const uint8_t ID_BITS = 36; // Allows up to 68 billion IDs
         static const uint8_t POS_BITS = 64 - ID_BITS;
-        static const uint64_t HIGH_MASK = ~0 << POS_BITS;
+        static const uint64_t HIGH_MASK = static_cast<uint64_t>(~0) << POS_BITS;
         static const uint64_t LOW_MASK = ~HIGH_MASK;
 };

The second change should only avoid possible problems and it should not give any issues on other compiler. On the contrary, I am not sure about the first change. Maybe it would be a good idea to use macros to switch between the std::unordered_map and the std::tr1::unordered_map.