Tim Armstrong has posted comments on this change. Change subject: IMPALA-2615: support [[nodiscard]] on Status ......................................................................
Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/7253/5/be/CMakeLists.txt File be/CMakeLists.txt: Line 73: if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0) > I find it is a confusing pattern. I don't actually see a reasonable alternative that doesn't involve extra redundant code or a lot of refactoring. The current approach is to build up the argument strings then switch on ${CMAKE_BUILD_TYPE} below to determine whether it's a GCC or Clang build. I added a comment to explain why it works. We can remove the branch and clean it up when we upgrade GCC. Line 74: # -Wno-placement-new: avoid spurious warnings from boost::function. > Are those warnings really spurious? I can't tell immediately from the link. It seems ok - the memory region written is large enough. The function_buffer union is large enough at 8 bytes, it's just the data member that is too small at 1 byte. Line 75: # TODO: remove when we upgrade Boost: https://github.com/boostorg/function/pull/9 > Thanks for looking into that. -faligned-new I think is fine, since it's enabling improved behaviour that is part of the C++17 standard instead of masking an issue. I don't think we should let upgrading Boost block upgrading GCC - I have a feeling that the Boost upgrade could be a lot of work. http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/statestore/statestore.h File be/src/statestore/statestore.h: Line 443: /// the > fix wrap Done http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/testutil/in-process-servers.cc File be/src/testutil/in-process-servers.cc: Line 71: Status status = impala->SetCatalogInitialized(); > const Done http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/parquet-reader.cc File be/src/util/parquet-reader.cc: Line 138: // Returns the number of rows specified by the header. > pre-existing issue, but: can you add a note that this function can exit(1) Done http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: Line 748: MakeScopeExitTrigger([&compressor]() { compressor->Close(); }); > Should Close() be added to the destructor (after making it unipotent)? We've been trying to standardise elsewhere on explicit resource release with trivial destructors. The argument is that implicit sequencing of non-trivial teardown steps is very hard to understand. I put up a page in the wiki that documents some of the patterns in the code: https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala http://gerrit.cloudera.org:8080/#/c/7253/6/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: Line 268: Status SerializeToArchiveString(std::string* out) const WARN_UNUSED_RESULT; > Makes the case for a Maybe<T> template class that can hold a T or a Status. I don't feel strongly. I like those types in other languages but they don't seem to work as well in C++ where there isn't a convenient language construct to branch on the result and unpack the value. We have to use LLVM's ErrorOr in a couple of places and IMO the code wasn't simpler than using an output argument. It looks like C++17 has std::optional, which is available earlier as std::experimental::optional http://en.cppreference.com/w/cpp/experimental/optional -- To view, visit http://gerrit.cloudera.org:8080/7253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
