[PATCH] D60547: Add checks for MSVC in LLVM_FALLTHROUGH and LLVM_NODISCARD

[Thad House via Phabricator via llvm-commits]

ThadHouse marked an inline comment as done.
ThadHouse added inline comments.


================
Comment at: llvm/include/llvm/Support/Compiler.h:123
 /// LLVM_NODISCARD - Warn if a type or return value is discarded.
 #if __cplusplus > 201402L && __has_cpp_attribute(nodiscard)
 #define LLVM_NODISCARD [[nodiscard]]
----------------
rnk wrote:
> ThadHouse wrote:
> > rnk wrote:
> > > ThadHouse wrote:
> > > > zturner wrote:
> > > > > zturner wrote:
> > > > > > What about just changing this `&&` to an `||`?
> > > > > Another question: Why is the check for `__cplusplus` even needed?
> > > > Is there CI for testing against old versions of clang and GCC, to make sure that it doesn't cause a regression. Otherwise yeah I think that would work too. Although I wonder if the original creators had a reason to check a newer c++ version.
> > > I think we can check `__has_cpp_attribute` without the `__cplusplus` check, or just check `defined(__cplusplus)` if we need to avoid C++ style attributes in C code. I'll test it and try committing that tomorrow.
> > For MSVC that might not be enough, as it errors if it falls through to the `gnu::***` and `llvm::***` attribute checks. For MSVC even in C++ mode, if `nodiscard` attribute is not found, the checks afterward can't happen either, as they error. And older versions of MSVC will likely error on just the initial `__has_cpp_attribute`, because isn't that new to the standard?
> We conditionally define `__has_cpp_attribute` earlier in this file for pre-c++17 compilers. I compiled this snippet with a variety of MSVC versions, and I think it does the right thing in all cases:
> ```
> #ifndef __has_cpp_attribute
> # define __has_cpp_attribute(x) 0
> #endif
> 
> #if __has_cpp_attribute(nodiscard)
> int case_a;
> #elif __has_cpp_attribute(clang::warn_unused_result)
> int case_b;
> #else
> int case_c;
> #endif
> ```
> I don't see intellisense warnings when I load that file in VS 2019 or 2017 either.
Hmm, mine complains about the clang:: part if I force the `nodiscard` to be false. Change `#if __has_cpp_attribute(nodiscard)` to `#if __has_cpp_attribute(nodiscarda)` to force the fallthrough and you'll see the error. And that is the case we don't want erroring, because that means nodiscard is not supported.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60547/new/

https://reviews.llvm.org/D60547