Added equality operator

[rajish]

Tried to use googlemock for tests and it appears that the comparison operator is necessary.

[mfontanini]

Could you post your compiler error that lead you to this PR? Also, what's your compiler? Tests shouldn't be using operator== as otherwise it wouldn't compile anywhere.

[rajish]

Here's the test code:

TEST_F(KafkaBrokerConfig, Flatten) {
    std::vector<cppkafka::ConfigurationOption> options;
    options = broker->flatten_options(broker->getConfig(), options, {"topic", "topics"});
    std::vector<cppkafka::ConfigurationOption> expected = {
        {"consumer.metadata.broker.list", "localhost"},
        {"producer.metadata.broker.list", "localhost"}
    };
    ASSERT_THAT(expected, ::testing::ElementsAreArray(options));
}

And here's the compiler:

-- The C compiler identification is AppleClang 8.1.0.8020042
-- The CXX compiler identification is AppleClang 8.1.0.8020042

And it's output:

In file included from /Users/rajish/proj/test/my_lib/test/kafka_broker_test.cpp:4:
In file included from /Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock.h:61:
In file included from /Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-generated-function-mockers.h:43:
In file included from /Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-spec-builders.h:75:
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:204:60: error: invalid operands to binary expression ('const cppkafka::ConfigurationOption' and 'const cppkafka::ConfigurationOption')
  bool operator()(const A& a, const B& b) const { return a == b; }
                                                         ~ ^  ~
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:908:14: note: in instantiation of function template specialization 'testing::internal::AnyEq::operator()<cppkafka::ConfigurationOption, cppkafka::ConfigurationOption>' requested here
      return Op()(lhs, rhs_);
             ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:905:14: note: in instantiation of member function 'testing::internal::ComparisonBase<testing::internal::EqMatcher<cppkafka::ConfigurationOption>, cppkafka::ConfigurationOption, testing::internal::AnyEq>::Impl<const cppkafka::ConfigurationOption &>::MatchAndExplain' requested here
    explicit Impl(const Rhs& rhs) : rhs_(rhs) {}
             ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:898:28: note: in instantiation of member function 'testing::internal::ComparisonBase<testing::internal::EqMatcher<cppkafka::ConfigurationOption>, cppkafka::ConfigurationOption, testing::internal::AnyEq>::Impl<const cppkafka::ConfigurationOption &>::Impl' requested here
    return MakeMatcher(new Impl<Lhs>(rhs_));
                           ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:3747:40: note: in instantiation of function template specialization 'testing::internal::ComparisonBase<testing::internal::EqMatcher<cppkafka::ConfigurationOption>, cppkafka::ConfigurationOption, testing::internal::AnyEq>::operator Matcher<const cppkafka::ConfigurationOption &>' requested here
Matcher<T>::Matcher(T value) { *this = Eq(value); }
                                       ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:555:12: note: in instantiation of member function 'testing::Matcher<const cppkafka::ConfigurationOption &>::Matcher' requested here
    return polymorphic_matcher_or_value;
           ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:531:12: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
    return CastImpl(
           ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:531:12: note: in instantiation of member function 'testing::internal::MatcherCastImpl<const std::__1::vector<cppkafka::ConfigurationOption, std::__1::allocator<cppkafka::ConfigurationOption> > &, testing::internal::ElementsAreArrayMatcher<cppkafka::ConfigurationOption> >::CastImpl' requested here
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:628:45: note: in instantiation of member function 'testing::internal::MatcherCastImpl<const std::__1::vector<cppkafka::ConfigurationOption, std::__1::allocator<cppkafka::ConfigurationOption> > &, testing::internal::ElementsAreArrayMatcher<cppkafka::ConfigurationOption> >::Cast' requested here
    return internal::MatcherCastImpl<T, M>::Cast(polymorphic_matcher_or_value);
                                            ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:666:34: note: in instantiation of function template specialization 'testing::SafeMatcherCastImpl<const std::__1::vector<cppkafka::ConfigurationOption, std::__1::allocator<cppkafka::ConfigurationOption> > &>::Cast<testing::internal::ElementsAreArrayMatcher<cppkafka::ConfigurationOption> >' requested here
  return SafeMatcherCastImpl<T>::Cast(polymorphic_matcher);
                                 ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:1854:39: note: in instantiation of function template specialization 'testing::SafeMatcherCast<const std::__1::vector<cppkafka::ConfigurationOption, std::__1::allocator<cppkafka::ConfigurationOption> > &, testing::internal::ElementsAreArrayMatcher<cppkafka::ConfigurationOption> >' requested here
    const Matcher<const T&> matcher = SafeMatcherCast<const T&>(matcher_);
                                      ^
/Users/rajish/proj/test/my_lib/test/kafka_broker_test.cpp:69:5: note: in instantiation of function template specialization 'testing::internal::PredicateFormatterFromMatcher<testing::internal::ElementsAreArrayMatcher<cppkafka::ConfigurationOption> >::operator()<std::__1::vector<cppkafka::ConfigurationOption, std::__1::allocator<cppkafka::ConfigurationOption> > >' requested here
    ASSERT_THAT(expected, ::testing::ElementsAreArray(options));
    ^
/Users/rajish/proj/test/googletest-1.8.0/googlemock/include/gmock/gmock-matchers.h:4388:37: note: expanded from macro 'ASSERT_THAT'
#define ASSERT_THAT(value, matcher) ASSERT_PRED_FORMAT1(\
                                    ^
/Users/rajish/proj/test/googletest-1.8.0/googletest/include/gtest/gtest_pred_impl.h:118:3: note: expanded from macro 'ASSERT_PRED_FORMAT1'
  GTEST_PRED_FORMAT1_(pred_format, v1, GTEST_FATAL_FAILURE_)
  ^
/Users/rajish/proj/test/googletest-1.8.0/googletest/include/gtest/gtest_pred_impl.h:101:28: note: expanded from macro 'GTEST_PRED_FORMAT1_'
  GTEST_ASSERT_(pred_format(#v1, v1), \
                           ^
/usr/local/include/cppkafka/buffer.h:164:6: note: candidate function not viable: no known conversion from 'const cppkafka::ConfigurationOption' to 'const cppkafka::Buffer' for 1st argument
bool operator==(const Buffer& lhs, const Buffer& rhs);
     ^
/Users/rajish/proj/test/googletest-1.8.0/googletest/include/gtest/internal/gtest-linked_ptr.h:223:6: note: candidate template ignored: could not match 'T *' against 'cppkafka::ConfigurationOption'
bool operator==(T* ptr, const linked_ptr<T>& x) {
     ^
1 error generated.
gmake[3]: *** [my_lib/test/CMakeFiles/mylib-test.dir/build.make:111: my_lib/test/CMakeFiles/mylib-test.dir/kafka_broker_test.cpp.o] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:353: my_lib/test/CMakeFiles/mylib-test.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:365: my_lib/test/CMakeFiles/mylib-test.dir/rule] Error 2
gmake: *** [Makefile:229: mylib-test] Error 2

[mfontanini]

So you added new code that relies on using operator==? If this is specific for your code, you can just add operator== as a free function on your project.

[rajish]

No, this a use case where the googlemock framework requires it. I don't use it in my code (yet).

[arvidn]

the operator should be a free function regardless, to support type conversions on both left and right side.

[rajish]

It will support both sides, because it's comparison against the ConfigurationOption. Objects on both sides are of the same type.

[mfontanini]

You normally define free functions so that if your type is implicitly convertible from something else, it also works when using that other type as the left hand side of the operator. e.g. if ConfigurationOption were constructible from just a name (value less options?) you wouldn't be able to do this with your solution:

ConfigurationOption config("some_config");
if ("some_config" == config) { // < fails to build
   
}

If you turn this to a free function and also provide operator!=, I'll merge it. Sorry about the delay on this one.

[arvidn]

There's a guideline about it, but it doesn't include the rationale as well as @mfontanini just did.

[accelerated]

Actually the operator<(const ConfigurationOption& lhs, const ConfigurationOption& rhs) should be implemented and all the other operators evolve from that. Once you have it, you can also put the ConfigurationOptions into an ordered container.

bool operator<(const ConfigurationOption& lhs, const ConfigurationOption& rhs) {
    if (lhs.key_ < rhs.key_) return true;
    if (rhs.key_ < lhs.key_) return false;
    return lhs.value_ < rhs.value_;
}

[accelerated]

also ConfigurationOption could have easily been implemented as
using ConfigurationOption = std::pair<std::string, std::string> or
using ConfigurationOption = std::pair<std::string, boost::any> for more flexibility
and then you would have all the operators for free, but it's too late now to make this kind of change.

[arvidn]

in my mind the rationale for adding the comparison operators is still a bit weak. "google mock requires it" is all I see so far. It's not a given that supporting more operators is better. The question that needs to be answered is: "does it make sense to compare configuration options?", and then specifically which kinds of comparisons make sense. (at just a brief look, it seems reasonable that operator== and operator!= make sense)

[mfontanini]

I agree with @arvidn. Does it really make sense for ConfigurationOption to be fully comparable? You can always use your own comparator objects for any specific use cases you need. If there's actually some where having some operators defined makes sense, then it's worth adding.

@accelerated using boost::any is not ideal here as you can't just store anything in each option.

[accelerated]

@mfontanini and @arvidn Yes my comment was not contending whether or not it's useful to have (I can't think of a use case for comparing options) but IF you have to add == and !=, it's better to create < and then use that one to build == and != (and you get container storable options at the same time).

operator==(a,b): !operator<(a,b) && !operator<(b,a)
operator!=(a,b): !operator==()

And we have to wait till c++20 for operator<=>() which would provide all these in one operation. Not to mention that comparison operators in this context would be best made to be case-insensitive.

[mfontanini]

Closing this as it's partially implemented at this point and it's stalled. If we're adding operator==, we should also add operator!=. Feel free to re-open a PR providing both of them if you feel like it.