rollbear / dry-comparisons

C++17 Utility classes for comparing multiple values in one simple expression
The Unlicense
204 stars 13 forks source link

include iosfwd instead iostream #3

Closed Neargye closed 4 years ago

rollbear commented 4 years ago

Did this work around the MSVC bug?

Neargye commented 4 years ago

No, just less depend on big iostream. I still try to understand msvc error msg.

rollbear commented 4 years ago

Back from the conference.

I don't think I understand this change. You get a compilation error if you #include "dry-comparisons.hpp" without having included <iostream> before it, which does not seem like an improvement to me. Am I missing something?

Neargye commented 4 years ago

The case when you need to use output operator is not so frequent. Most likely people want to include dry-comparisons and use like state == any_of(S1, S2, S3), and they not need operator <<, but if iostream included they will get slow compilation.

Neargye commented 4 years ago

If really need an output operator, first include iostream then dry-comparisons.

#include <iostream>
#include "dry-comparisons.hpp"
rollbear commented 4 years ago

No, it is not about needing it. Just this program fails to compile:

// t.cpp
#include "dry-comparisons.hpp"
bf😱confuciusornis /tmp> /opt/clang-9/bin/clang++   -std=c++17 -I ~/develop/dry-comparisons t.cpp
In file included from t.cpp:1:
/home/bf/develop/dry-comparisons/dry-comparisons.hpp:68:12: error: invalid operands to binary expression ('std::ostream' (aka 'basic_ostream<char>') and 'const char *')
        os << label << '{';
        ~~ ^  ~~~~~
bf😱confuciusornis /tmp> clang++-8   -std=c++17 -I ~/develop/dry-comparisons t.cpp
In file included from t.cpp:1:
/home/bf/develop/dry-comparisons/dry-comparisons.hpp:68:12: error: invalid operands to binary
      expression ('std::ostream' (aka 'basic_ostream<char>') and 'const char *')
        os << label << '{';
        ~~ ^  ~~~~~
bf😱confuciusornis /tmp> g++-9   -std=c++17 -I ~/develop/dry-comparisons t.cpp
In file included from t.cpp:1:
/home/bf/develop/dry-comparisons/dry-comparisons.hpp: In member function ‘std::ostream& rollbear::internal::logical_tuple<Ts>::print(const char*, std::ostream&) const’:
/home/bf/develop/dry-comparisons/dry-comparisons.hpp:68:12: error: no match for ‘operator<<’ (operand types are ‘std::ostream’ {aka ‘std::basic_ostream<char>’} and ‘const char*’)
   68 |         os << label << '{';
      |         ~~ ^~ ~~~~~
      |         |     |
      |         |     const char*
      |         std::ostream {aka std::basic_ostream<char>}

I do note that MSVC does accept the code by default, but if you compile with /permissive- it too fails to compile with a similar error message.

Neargye commented 4 years ago

Now I see, I apologize, carried away by an attempt to start on Windows. (By the way, it turned out to restore part of the functionality.)

Neargye commented 4 years ago

now build ok https://wandbox.org/permlink/ZvdLvarIdXuUdlfq

rollbear commented 4 years ago

Sure, but <string_view> includes <iostream>, so you have not gained anything.

Neargye commented 4 years ago

String_view include iosfrd https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/string_view

rollbear commented 4 years ago

Um, interesting. It pulls in about 50kLOC of headers, much of which is iostream stuff, but <iostream> itself pulls in 90kLOC of headers, so it is a noticeable saving. I like that. Thanks.

Neargye commented 4 years ago

I would be glad to meet in person in St. Petersburg. Maybe I will fix msvc by this time)

Neargye commented 4 years ago

Will try remove depend on string_view, maybe help more.

rollbear commented 4 years ago

Yeah, <string_view> is actually not needed, and the test program can (and should) include "dry-comparisons.hpp" before any other headers. I slightly modified your version like this:

    template <typename C>
    std::basic_ostream<C>& print(const char* label, std::basic_ostream<C>& os) const
    {
        os << label << '{';
        std::apply([&os](const auto& ... v) {
            int first = 1;
            ((os << &","[std::exchange(first,0)] << v),...);
        }, self());
        return os << '}';

Now os is a dependent name, so it all seems to work fine.

Thanks. This was a bit of an unexpected adventure. A good one.

Meet in St. Petersburg sounds good. Going to the conf?

Neargye commented 4 years ago

Maybe print(const C* label...)?

Neargye commented 4 years ago

Yes, planned go to conf.

Neargye commented 4 years ago

Becouse steam can't print char uses <<.

rollbear commented 4 years ago

Yeah, const C* label would be better.

Neargye commented 4 years ago

Update PR.

rollbear commented 4 years ago

Super! Thanks.