Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status

Patch Set 6:


File be/CMakeLists.txt:

> 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: 
> 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.

File be/src/statestore/statestore.h:

Line 443:   /// the
> fix wrap

File be/src/testutil/in-process-servers.cc:

Line 71:       Status status = impala->SetCatalogInitialized();
> const

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) 

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: 

File be/src/util/runtime-profile.h:

Line 268:   Status SerializeToArchiveString(std::string* out) const 
> 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 

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

Reply via email to