Lars Volker has posted comments on this change. Change subject: IMPALA-5425: Add test for validating input when setting query options ......................................................................
Patch Set 6: (9 comments) Thank you for working on this. I left some comments inline. Overall I feel that we're still not completely decided on which new features of C++11 and new paradigms they allow we want to use. I'd be happy to follow up on this on dev@. http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: Line 20: #include <boost/fusion/algorithm/iteration/for_each.hpp> We are generally trying to reduce our dependencies on Boost libraries. Can you replace some of these with C++11 equivalents (e.g. for_each))? Line 39: constexpr auto i32_max = numeric_limits<int32_t>::max(); We currently use auto only for iterators and const& in loops. Line 73: TEST(QueryOptions, SetByteOptions) { Can you add a comment explaining what the test does overall? Same for the other tests. Line 96: auto process_case_set = [&](auto& case_set) { I find this part hard to follow and its use of lambdas seems to deviate from our current best practices somewhat. Can you try to refactor some parts using plain functions to generate context objects instead? PS6, Line 97: kase Can you think of a better name? PS6, Line 99: minus_1_to_0 Why is this needed? Is the added level of indirection worth it? PS6, Line 137: // Expands to {"entry", prefix::entry}, : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), prefix::entry}, : // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", prefix::b}, : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, prefix, name) : // A case is a pair: (keydef, [(enum_string, enum_value)]) : #define CASE(key, enumtype, enums) make_pair(key, \ : vector<pair<const char*, enumtype::type>>\ : {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))}) I find this very hard to understand. Can you have a look at the Google Test documentation on how to define test cases? Line 171: fusion::for_each(case_set, [&options, accept_integer](auto& kase) { Can you use C++11 for_each instead? Line 263: for (auto& key_def : {key_def_min, key_def_default}) { const auto&? -- To view, visit http://gerrit.cloudera.org:8080/7805 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-HasComments: Yes
