Re: Preferred syntax for warning about ignored Status returns

2017-01-10 Thread Tim Armstrong
It doesn't seem like there's a clear consensus - I counted two people in favour of Option 1 and three in favour of Option 2 (one weakly). I'll probably go forward with Option 2 because other projects that we (now or in future) pull in source from (Kudu, gutil) use it. On Mon, Jan 9, 2017 at 10:33

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Zachary Amsden
Yes Jim, exactly. I also sent the wrong link at least once, but this was my point. In the absence of compiler support I think it is actually possible to implement this anyway using syntax 1 but that is left as an exercise to the reader. On Mon, Jan 9, 2017 at 10:20 AM, Jim Apple wrote: > Thoug

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Jim Apple
Though the attribute is "on" the definition, it can't appear in that location (after parameters, before curly braces). I don't know why. I think I now understand what you were saying above: if we use WARN_UNUSED_RESULT, then it can go after the function for functions with prototypes but must go ea

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Tim Armstrong
Zach - looks like you're right, GCC seems to allow the second syntax only on function forward declarations, not definitions. On Mon, Jan 9, 2017 at 10:11 AM, Zachary Amsden wrote: > Maybe I'm just being dense but I couldn't get it to work with syntax 2 on a > function definition without having a

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Zachary Amsden
Maybe I'm just being dense but I couldn't get it to work with syntax 2 on a function definition without having a separate forward declaration: https://godbolt.org/g/ODtoQC vs. https://godbolt.org/g/WCxDZv (non-functional) - Zach On Mon, Jan 9, 2017 at 10:00 AM, Jim Apple wrote: > That's appl

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Jim Apple
That's applying it to the type definition. At the type use: https://godbolt.org/g/RMYVW7 On Mon, Jan 9, 2017 at 9:56 AM, Zachary Amsden wrote: > GCC doesn't catch this when optimization is enabled and the result is > discarded: > > https://godbolt.org/g/4b0BQC > > I think that means a type wrapp

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Zachary Amsden
GCC doesn't catch this when optimization is enabled and the result is discarded: https://godbolt.org/g/4b0BQC I think that means a type wrapper approach is needed, which probably necessitates option 1. - Zach On Mon, Jan 9, 2017 at 9:17 AM, Jim Apple wrote: > My vote, as I mentioned on the p

Re: Preferred syntax for warning about ignored Status returns

2017-01-09 Thread Jim Apple
My vote, as I mentioned on the patch, is option 1. I see MUST_USE(T) as a property of T, like const T or volatile T. I think it is dual to move semantics or to Rust's ability to temporarily or permanently consume values so that /only/ one copy is in use rather than MUST_USE's /at least one/. https

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Taras Bobrovytsky
I'd vote for #2. I think it's better to have less important information (such as qualifiers) towards the end of lines. (I think it would be nice if modifiers such as public and private were at the end of method declarations in Java, for example: void myMethod() private static {...}) On Fri, Jan 6,

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Daniel Hecht
As I indicated in the original review, my preference is #2 but I don't feel too strongly. On Fri, Jan 6, 2017 at 2:59 PM, Tim Armstrong wrote: > Hi All, > I wanted to poll the Impala community for opinions about style for > declaring functions where the caller is expected to do something with

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Marcel Kornacker
I'd vote for option 2, which is visually less distracting and still communicates the same thing (in the end, it's a function qualifier). On Fri, Jan 6, 2017 at 2:59 PM, Tim Armstrong wrote: > Hi All, > I wanted to poll the Impala community for opinions about style for > declaring functions wher

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Todd Lipcon
On Fri, Jan 6, 2017 at 3:14 PM, Jim Apple wrote: > > todd@todd-ThinkPad-T540p:/tmp$ g++ --version ; g++ -o test test.cc > > g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609 > > > > test.cc:2:1: warning: ‘warn_unused_result’ attribute only applies to > > function types [-Wattributes] > > Foo {

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Jim Apple
> todd@todd-ThinkPad-T540p:/tmp$ g++ --version ; g++ -o test test.cc > g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609 > > test.cc:2:1: warning: ‘warn_unused_result’ attribute only applies to > function types [-Wattributes] > Foo { > ^ This seems like a warning stating that it doesn't work.

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Todd Lipcon
FWIW even though applying WARN_UNUSED_RESULT to a type is a C++17 extension, it works fine in C++11 on recent versions of Clang and gcc: todd@todd-ThinkPad-T540p:/tmp$ cat test.cc struct __attribute__((warn_unused_result)) Foo { }; Foo bar() { return Foo(); } int main() { bar(); return 0;

Re: Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Thomas Tauber-Marshall
I'd vote for option 1, given that, as you say, this is closely related to the return type, which putting the macro with the type makes clear. Also looking at buffer-pool.h in the review, it doesn't look overly noisy to me. On Fri, Jan 6, 2017 at 2:59 PM Tim Armstrong wrote: > Hi All, > I want

Preferred syntax for warning about ignored Status returns

2017-01-06 Thread Tim Armstrong
Hi All, I wanted to poll the Impala community for opinions about style for declaring functions where the caller is expected to do something with the return value. Ideally we'd be able to declare Status with an attribute that made this take effect globally, but unfortunately that's not available