Add checks for MSVC in LLVM_FALLTHROUGH and LLVM_NODISCARD

[ThadHouse]

Summary

MSVC defaults __cplusplus to an old version, which causes the preprocessor to fallthrough to the clang::xx attribute checks. MSVC see's these and errors intellisense. This checks for MSVC explicitly, and handles the attributes for it separately.

[zturner]

llvm/include/llvm/Support/Compiler.h

123

What about just changing this && to an ||?

[zturner]

llvm/include/llvm/Support/Compiler.h

123

Another question: Why is the check for __cplusplus even needed?

[ThadHouse]

llvm/include/llvm/Support/Compiler.h

123

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.

[rnk]

llvm/include/llvm/Support/Compiler.h

123

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.

[ThadHouse]

llvm/include/llvm/Support/Compiler.h

123

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?

[rnk]

llvm/include/llvm/Support/Compiler.h

123

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.

[ThadHouse]

llvm/include/llvm/Support/Compiler.h

123

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.

llvm/include/llvm/Support/Compiler.h

 /// LLVM_NODISCARD - Warn if a type or return value is discarded.
 #if __cplusplus > 201402L && __has_cpp_attribute(nodiscard)
 #define LLVM_NODISCARD [[nodiscard]]
+// Detect MSVC directly, since __cplusplus still defaults to old version
+#elif _MSVC_LANG >= 201703L
+#define LLVM_NODISCARD [[nodiscard]]
+#elif _MSC_VER
+#define LLVM_NODISCARD
 #elif !__cplusplus
 // Workaround for llvm.org/PR23435, since clang 3.6 and below emit a spurious
 // error when __has_cpp_attribute is given a scoped attribute in C mode.

 /// LLVM_FALLTHROUGH - Mark fallthrough cases in switch statements.
 #if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
 #define LLVM_FALLTHROUGH [[fallthrough]]
+// Detect MSVC directly, since __cplusplus still defaults to old version
+#elif _MSVC_LANG >= 201703L
+#define LLVM_FALLTHROUGH [[fallthrough]]
+#elif _MSC_VER
+#define LLVM_FALLTHROUGH
 #elif __has_cpp_attribute(gnu::fallthrough)
 #define LLVM_FALLTHROUGH [[gnu::fallthrough]]
 #elif !__cplusplus