Yingchun Lai 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: (10 comments) 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 Done 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. Done 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. Y Done 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 Done 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. Done http://gerrit.cloudera.org:8080/#/c/13294/5/src/kudu/util/yamlreader.h@77 PS5, Line 77: const std::string& field, : T* result) { > Indentation. Done 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? Done http://gerrit.cloudera.org:8080/#/c/13294/5/thirdparty/build-definitions.sh@932 PS5, Line 932: cmake \ > Pass in EXTRA_CMAKE_FLAGS too. Done 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. Done 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? No, because the original downloaded package name is like: yaml-cpp-yaml-cpp-0.6.2.tar.gz -- 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: Tue, 14 May 2019 10:10:29 +0000 Gerrit-HasComments: Yes
