Adding [[nodiscard]] to Compiler.h

I started to try to use llvm::Error recently. This has nice runtime
checks for if you didn't check the result, but I thought it would be
really nice to get a compiler warning for the obvious cases of this
rather than having to wait for a runtime check.

This, of course, is exactly what the C++17 [[nodiscard]] attribute is
for - with new enough compilers in C++17 mode we can just declare the
class like so:

  class [[nodiscard]] Error { ... };

So, I'd like to add an LLVM_NODISCARD macro to Compiler.h, and this is
where it gets interesting. Pre-C++17, clang already allows
__attribute__((warn_unused_result)) and [[clang::warn_unused_result]] to
be used on a class this way, with equivalent affects, so it'd be nice to
use that if we aren't building in C++17 mode.

We already have a LLVM_UNUSED_RESULT defined to this, but AFAICT gcc
only allows it on function declarations, and the MSVC equivalent
(_Check_return_) seems like it's not allowed on types either. I guess we
want a new macro LLVM_NODISCARD, that's [[nodiscard]] if available, else
[[clang::warn_unused_result]] if we're clang (maybe even clang 3.6 and
up, I'm not sure if it works before that), else empty.

Thoughts?

Hi Justin,

This SGTM generally, but please make the difference between
LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :slight_smile:

However, we have to make a policy decision here: if I have a
LLVM_NODISCARD "Error" class should functions that return an instance
of Error _also_ be annotated with LLVM_UNUSED_RESULT? IOW, how much
repetition are we willing to live with to get good diagnostics on
non-clang compilers?

I personally would be happier with _not_ annotating functions return
Error with LLVM_UNSUED_RESULT, since the clang bots would catch
violations anyway, and the code will look slightly neater.

Thanks,
-- Sanjoy

Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Hi Justin,

This SGTM generally, but please make the difference between
LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :slight_smile:

Right, this is where it gets a little weird. LLVM_NODISCARD would be for
types, whereas LLVM_UNUSED_RESULT would be for functions. Depending on
your host compiler, using the wrong one might DTRT, but it won't across
all compilers.

Do you think documenting this is sufficient, or should we try to name
these to better represent where they should be used?

However, we have to make a policy decision here: if I have a
LLVM_NODISCARD "Error" class should functions that return an instance
of Error _also_ be annotated with LLVM_UNUSED_RESULT? IOW, how much
repetition are we willing to live with to get good diagnostics on
non-clang compilers?

I personally would be happier with _not_ annotating functions return
Error with LLVM_UNUSED_RESULT, since the clang bots would catch
violations anyway, and the code will look slightly neater.

I'm also in favour of not annotating the functions - especially given a
case like Error (and it's friend, Expected<>). Annotating all of the
functions would add a lot of boilerplate for little gain.

Hi Justin,

Justin Bogner wrote:
> Sanjoy Das<sanjoy@playingwithpointers.com> writes:
>> Hi Justin,
>>
>> This SGTM generally, but please make the difference between
>> LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :slight_smile:
>
> Right, this is where it gets a little weird. LLVM_NODISCARD would be for
> types, whereas LLVM_UNUSED_RESULT would be for functions. Depending on
> your host compiler, using the wrong one might DTRT, but it won't across
> all compilers.
>
> Do you think documenting this is sufficient, or should we try to name
> these to better represent where they should be used?

Firstly, perhaps "LLVM_NODISCARD_TYPE" would be a better name?

Secondly, if we define the following when the host compiler is clang,

#define LLVM_UNUSED_RESULT __attribute__((warn_unused_result))
#define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]]

via some shallow manual testing, it looks like using
LLVM_NODISCARD_TYPE on a function should break the build (though I'd
love it if someone more familiar with clang chimed in on this). So, is
the problem that we can accidentally use LLVM_UNUSED_RESULT on a type
and not know it?

I think there is a dirty trick here -- we could:

#define LLVM_UNUSED_RESULT __attribute__((warn_unused_result, enable_if(true, "")))
#define LLVM_NODISCARD_TYPE [[clang::warn_unused_result]]

This breaks the (clang) build if LLVM_UNUSED_RESULT is used on a type.

It would still be possible to not do the right thing on GCC or MSVC
and be able to build a binary, but I think as long as the clang bots
catch bad uses we're okay.

-- Sanjoy`

My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD.

For compilers that support it, it should be a strict superset of features and functionality. The standard feature was written directly based on the clang warn_unused_result stuff.

I would just migrate us onto the spelling and usage pattern that got standardized. All we have to lose are warnings from compilers other than Clang until they implement this feature. That seems like not a huge loss to me honestly.

-Chandler

Hi Chandler,

Chandler Carruth wrote:
> My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD.
>
> For compilers that support it, it should be a strict superset of features and functionality. The standard feature was
> written directly based on the clang warn_unused_result stuff.

Maybe I misunderstood something, but with clang 3.8.1 I cannot write:

int [[clang::nodiscard]] f();

so I'm not sure if LLVM_NODISCARD provides a superset of the functionality.

-- Sanjoy

I don’t know anything about [[clang::nodiscard]], but I would expect [[clang::warn_unused_result]] in C++11 and C++14 to work in all the places and have the same effect as [[nodiscard]] does in C++17. Here is a recent clang build:

echo -e ‘[[nodiscard]] int f();\nstruct [[nodiscard]] X {};’ | clang -fsyntax-only -x c++ -std=c++1z -

echo -e ‘[[clang::warn_unused_result]] int f();\nstruct [[clang::warn_unused_result]] X {};’ | clang -fsyntax-only -x c++ -std=c++11 -

-Chandler

Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Hi Chandler,

Chandler Carruth wrote:

My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD.

For compilers that support it, it should be a strict superset of
features and functionality. The standard feature was
written directly based on the clang warn_unused_result stuff.

Maybe I misunderstood something, but with clang 3.8.1 I cannot write:

int [[clang::nodiscard]] f();

That's the wrong spot - the attribute would apply to `int` in this
expression. Also there's no [[clang::nodiscard]], it's either
[[nodiscard]] or [[clang::warn_unused_result]].

Thus, you spell it like so:

  [[nodiscard]] int f();

so I'm not sure if LLVM_NODISCARD provides a superset of the functionality.

As Chandler pointed out, [[nodiscard]] is almost exactly
[[clang::warn_unused_result]], which is itself the same as clang's
implementation of __attribute__((__warn_unused_result__)) modulo C++11
attributes being a bit more strict in terms of placement than gcc
attributes.

Chandler Carruth <chandlerc@google.com> writes:

My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD.

For compilers that support it, it should be a strict superset of features and
functionality. The standard feature was written directly based on the clang
warn_unused_result stuff.

I would just migrate us onto the spelling and usage pattern that got
standardized. All we have to lose are warnings from compilers other than Clang
until they implement this feature. That seems like not a huge loss to me
honestly.

You might be right. It seems a bit unfortunate that we'd lose these
warnings on MSVC and gcc for the time being, but maybe that's better
than the usability cost of having two macros that expand to the same
thing.

Do note though, if we do that this would mean clang is the only compiler
we can support this on at all without -std=c++17, since [[nodiscard]]
will trigger extension warnings in earlier standard modes.

Yea, I dunno.

If you end up wanting two macros, I’d suggest:

LLVM_NODISCARD

which works exactly as [[nodiscard]], and

LLVM_NODISCARD_FUNCTION

which works the way our current macro works. My two cents.

Justin Bogner <mail@justinbogner.com> writes:

Chandler Carruth <chandlerc@google.com> writes:

My 2 cents: get rid of LLVM_UNUSED_RESULT, and move to LLVM_NODISCARD.

For compilers that support it, it should be a strict superset of features and
functionality. The standard feature was written directly based on the clang
warn_unused_result stuff.

I would just migrate us onto the spelling and usage pattern that got
standardized. All we have to lose are warnings from compilers other than Clang
until they implement this feature. That seems like not a huge loss to me
honestly.

You might be right. It seems a bit unfortunate that we'd lose these
warnings on MSVC and gcc for the time being, but maybe that's better
than the usability cost of having two macros that expand to the same
thing.

Do note though, if we do that this would mean clang is the only compiler
we can support this on at all without -std=c++17, since [[nodiscard]]
will trigger extension warnings in earlier standard modes.

After talking to a few people it sounds like one LLVM_NODISCARD macro is
the way to go - We have prior art for clang-only warnings; we have
enough coverage building with clang that we'll still catch anything
quickly; and having two macros that are so similar is confusing and
difficult to use.

I'll stage this as adding an LLVM_NODISCARD, converting in tree code to
use it instead of LLVM_ATTRIBUTE_UNUSED_RESULT, then finally removing
the old macro.

Thanks very much Justin. This is great work in general, and it’ll be a huge improvement for llvm::Error users.

  • Lang.