[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-11-07 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 2:

I was going to poll a few people but this got bumped down my priority list a 
bit - will come back to it probably in a week or so.

-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-11-07 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 2:

> (1 comment)

Any updates no this?

-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/2/be/src/bufferpool/buffer-allocator.h
File be/src/bufferpool/buffer-allocator.h:

PS2, Line 38: MUST_USE(Status)
> The type attribute would definitely make it easier to apply, I'm ok with ho
I don't think we need to hold off.  Let's just decide on the syntax and then 
revisit once we can apply to the type.


-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/2/be/src/bufferpool/buffer-allocator.h
File be/src/bufferpool/buffer-allocator.h:

PS2, Line 38: MUST_USE(Status)
> I guess we'd have to wait for C++17, where in the link Tim sent (http://en.
Good point!

I guess we could pass "c++1z" rather than "c++14", but we might also need to 
upgrade to a newer gcc in the toolchain:

https://gcc.gnu.org/projects/cxx-status.html#cxx1z

Another idea is to switch to clang for all builds. I tested it, and clang 
allows this attribute on types:

struct[[gnu::warn_unused_result]] Foo {
  int bar;
};

Foo Baz() { return {11}; }

int main() {
  Baz();
  const auto qux = Baz();   

  return qux.bar;
}

GCC (4.9.2) warns about using the attribute in the wrong place. Clang 
(3.8.0-p1) warns about the unused return value from the Baz call on the first 
line of main.


-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/2/be/src/bufferpool/buffer-allocator.h
File be/src/bufferpool/buffer-allocator.h:

PS2, Line 38: MUST_USE(Status)
> I would prefer MUST_USE(x) better as an API user, since I feel it gives me 
I guess we'd have to wait for C++17, where in the link Tim sent 
(http://en.cppreference.com/w/cpp/language/attributes) it appears to become 
usable as a type attribute.


-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4878/1//COMMIT_MSG
Commit Message:

Line 14: The macro is only used in the buffer pool for now, but we can use it in
> I saw a JIRA you filed about this where you found a likely bug. Maybe refer
Done


http://gerrit.cloudera.org:8080/#/c/4878/1/be/src/common/status.h
File be/src/common/status.h:

PS1, Line 262: __attribute__((warn_unused_result))
> This can be [[gnu::warn_unused_result]] with the new C++11 syntax
Done. Looks like eventually this will be standardised as [[nodiscard]] 
http://en.cppreference.com/w/cpp/language/attributes


Line 262: #define STATUS_RETURN __attribute__((warn_unused_result)) Status
> How would you feel about making this a function-like macro, like MUST_USE(T
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change.

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4878/1//COMMIT_MSG
Commit Message:

Line 14: The macro is only used in the buffer pool for now, but we can use it in
I saw a JIRA you filed about this where you found a likely bug. Maybe reference 
that here?


http://gerrit.cloudera.org:8080/#/c/4878/1/be/src/common/status.h
File be/src/common/status.h:

Line 262: #define STATUS_RETURN __attribute__((warn_unused_result)) Status
How would you feel about making this a function-like macro, like MUST_USE(Type) 
__attribute__((warn_unused_result)) Type?


PS1, Line 262: __attribute__((warn_unused_result))
This can be [[gnu::warn_unused_result]] with the new C++11 syntax


-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3200 (buffer pool): warn if Status is ignored

2016-10-28 Thread Tim Armstrong (Code Review) 
Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4878

Change subject: IMPALA-3200 (buffer pool): warn if Status is ignored
..

IMPALA-3200 (buffer pool): warn if Status is ignored

This introduces a STATUS_RETURN macro that expands to
__attribute__((warn_unused_result)) Status. It can be used in function
declarations in place of Status to issue warnings if a return Status
is ignored.

The macro is only used in the buffer pool for now, but we can use it in
other places in follow-up patches if we agree on the pattern.

Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
---
M be/src/bufferpool/buffer-allocator.h
M be/src/bufferpool/buffer-pool.h
M be/src/common/status.h
3 files changed, 16 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/4878/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4878
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong