Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13294 )
Change subject: [util] Introduce yaml-cpp to read config files ...................................................................... Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/13294/5/cmake_modules/FindYaml.cmake File cmake_modules/FindYaml.cmake: http://gerrit.cloudera.org:8080/#/c/13294/5/cmake_modules/FindYaml.cmake@22 PS5, Line 22: find_library(YAML_STATIC_LIB libyaml-cpp.a Could we also generate a shared library version of yaml-cpp to use for dynamically linked builds? http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc File src/kudu/util/yamlreader-test.cc: http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@44 PS5, Line 44: Nit: space here http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@45 PS5, Line 45: Env::Default() KuduTest has an env_ you can use. http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@66 PS5, Line 66: ASSERT_TRUE(r->ExtractScalar(r->node(), "foo", &val).IsCorruption()); The Extract functions are static, so shouldn't we call them as such? i.e. YamlReader::ExtractScalar(...). http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader-test.cc@75 PS5, Line 75: ASSERT_TRUE(r->ExtractScalar(r->node(), "foo", &val).IsCorruption()); Shouldn't this return something IsNotFound since 'foo' doesn't exist in the file? http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h File src/kudu/util/yamlreader.h: http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h@39 PS5, Line 39: class YamlReader { Please doc the class and its public methods so people know how to use it. http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h@77 PS5, Line 77: const std::string& field, : T* result) { Indentation. http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@928 PS5, Line 928: CFLAGS="$EXTRA_CFLAGS -fPIC" \ : CXXFLAGS="$EXTRA_CXXFLAGS -fPIC" \ Why do we need to explicitly add -fPIC? http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@932 PS5, Line 932: cmake \ Pass in EXTRA_CMAKE_FLAGS too. http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@939 PS5, Line 939: make -j$PARALLEL $EXTRA_MAKEFLAGS install Use ${NINJA:-make} here to prefer ninja if it exists. http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/vars.sh File thirdparty/vars.sh: http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/vars.sh@239 PS5, Line 239: yaml-cpp-yaml-cpp- Why is yaml-cpp listed twice? Can we remove one? -- To view, visit http://gerrit.cloudera.org:8080/13294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ef58befaffbcc880e13fa6fec61b8e94a189b5a Gerrit-Change-Number: 13294 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Mon, 13 May 2019 19:41:07 +0000 Gerrit-HasComments: Yes
