[llvm-dev] Adding [[nodiscard]] to Compiler.h

[Justin Bogner via llvm-dev]

Sanjoy Das <sanjoy at playingwithpointers.com> writes:
> Hi Justin,
>
> This SGTM generally, but please make the difference between
> LLVM_NODISCARD and LLVM_UNUSED_RESULT clear in the code. :)

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.

> Thanks,
> -- Sanjoy
>
>
> On Fri, Sep 2, 2016 at 2:20 PM, Justin Bogner via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> 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?
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev