impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Armstrong <tarmstr...@cloudera.com>
Subject Re: Preferred syntax for warning about ignored Status returns
Date Tue, 10 Jan 2017 22:42:22 GMT
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 AM, Zachary Amsden <zamsden@cloudera.com>
wrote:

> 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 <jbapple@cloudera.com> wrote:
>
> > 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 earlier in the line for functions without
> > prototypes.
> >
> > Did I get that right?
> >
> > On Mon, Jan 9, 2017 at 10:11 AM, Zachary Amsden <zamsden@cloudera.com>
> > 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 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 <jbapple@cloudera.com>
> wrote:
> > >
> > >> 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 <zamsden@cloudera.com>
> > >> 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 wrapper approach is needed, which probably
> > >> > necessitates option 1.
> > >> >
> > >> >  - Zach
> > >> >
> > >> > On Mon, Jan 9, 2017 at 9:17 AM, Jim Apple <jbapple@cloudera.com>
> > wrote:
> > >> >
> > >> >> 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://en.wikipedia.org/wiki/Substructural_type_system
> > >> >>
> > >> >> On Fri, Jan 6, 2017 at 4:05 PM, Taras Bobrovytsky
> > >> >> <tbobrovytsky@cloudera.com> wrote:
> > >> >> > 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, 2017 at 3:38 PM, Daniel Hecht <
> dhecht@cloudera.com>
> > >> >> wrote:
> > >> >> >
> > >> >> >> 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 <
> > >> tarmstrong@cloudera.com>
> > >> >> >> 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
> > >> >> 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
> > until
> > >> >> C++17.
> > >> >> >> >
> > >> >> >> > So we need to annotate each Status-returning function.
The two
> > >> >> >> alternatives
> > >> >> >> > we discussed on this CR (https://gerrit.cloudera.org/#
> /c/4878/)
> > >> were:
> > >> >> >> >
> > >> >> >> > #1 - a special macro wrapping Status
> > >> >> >> >
> > >> >> >> > MUST_USE(Status) DoSomethingThatCanFail(int64_t
foo, Bar*
> bar);
> > >> >> >> >
> > >> >> >> > Pros:
> > >> >> >> > * Closely connected to the return type that it affects
> > >> >> >> > * It's easier to search/replace Status with MUST_USE(Status)
> > >> >> >> >
> > >> >> >> > Cons:
> > >> >> >> > * Could get visually noisy if we use it everywhere
> > >> >> >> >
> > >> >> >> > #2 - a macro that gets appended to the declaration:
> > >> >> >> >
> > >> >> >> > Status DoSomethingThatCanFail(int64_t foo, Bar*
bar)
> > >> >> WARN_UNUSED_RESULT;
> > >> >> >> >
> > >> >> >> > Pros:
> > >> >> >> > * Macro is slightly
> > >> >> >> > * Less visually noisy since it's at the end of the
declaration
> > >> >> >> >
> > >> >> >> > What do people think?
> > >> >> >> >
> > >> >> >>
> > >> >>
> > >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message