Fix error in Visual Studio due to __has_cpp_attribute

[haozhun]

Summary

Visual Studio introduced __has_cpp_attributed in 15.8. However, Visual
Studio IntelliSense does not allow :: in __has_cpp_attributed, and
produces error "E2512 the argument to a feature-test macro must be a
simple identifier" for code like #if __has_cpp_attribute(clang::...).

The VS compiler does not produce this error. The error is only produced
by VS IntelliSense. As a result, without this commit, although the code
produces an error in Visual Studio development environment, it does not
make the actual build fail.

[haozhun]

I'm new to LLVM. I found the reviewer using blame and CODE_OWNERS.TXT. Please let me know if you are not the appropriate reviewer, @chandlerc.

[aaron.ballman]

Visual Studio introduced has_cpp_attributed in 15.8. However, Visual
Studio IntelliSense does not allow :: in has_cpp_attributed, and
produces error "E2512 the argument to a feature-test macro must be a
simple identifier" for code like #if __has_cpp_attribute(clang::...).

This is a Visual Studio bug with their IntelliSense implementation. http://eel.is/c++draft/cpp.cond#5 says the pp-tokens are to be interpreted as an attribute-token, which includes attribute-scoped-tokens.

The VS compiler does not produce this error. The error is only produced
by VS IntelliSense. As a result, without this commit, although the code
produces an error in Visual Studio development environment, it does not
make the actual build fail.

IntelliSense squiggly lines often do not match the compiler behavior (IntelliSense uses a different frontend than cl, from what I understand); why do we need to work around it here? I guess I don't understand what problem this is solving given that the compilation succeeds.

[haozhun]

Thank you for your review.

This is a Visual Studio bug with their IntelliSense implementation. http://eel.is/c++draft/cpp.cond#5 says the pp-tokens are to be interpreted as an attribute-token, which includes attribute-scoped-tokens.

Thank you for pointing this out and providing citations. I understand that Windows is a supported platform of LLVM. As a result, I suppose it doesn't matter whose fault this is.

IntelliSense squiggly lines often do not match the compiler behavior (IntelliSense uses a different frontend than cl, from what I understand); why do we need to work around it here? I guess I don't understand what problem this is solving given that the compilation succeeds.

If there isn't already a policy/consensus on whether IntelliSense errors should be avoided (especially in header files), it would be a judgement call.

However, I would expect that the choice had been made in the past. If IntelliSense misbehavior are indeed "often" (as you said), and given that there aren't other IntelliSense complaints of header files in the code base (as far as I can tell), it is likely that a choice had been made in the past that IntelliSense errors should be avoided.

[aaron.ballman]

In D57196#1374340, @haozhun wrote:

Thank you for your review.

This is a Visual Studio bug with their IntelliSense implementation. http://eel.is/c++draft/cpp.cond#5 says the pp-tokens are to be interpreted as an attribute-token, which includes attribute-scoped-tokens.

Thank you for pointing this out and providing citations. I understand that Windows is a supported platform of LLVM. As a result, I suppose it doesn't matter whose fault this is.

Sort of. We should make sure the issue gets reported to Microsoft, and if it causes us tangible pain, we could work around it locally.

IntelliSense squiggly lines often do not match the compiler behavior (IntelliSense uses a different frontend than cl, from what I understand); why do we need to work around it here? I guess I don't understand what problem this is solving given that the compilation succeeds.

If there isn't already a policy/consensus on whether IntelliSense errors should be avoided (especially in header files), it would be a judgement call.

To the best of my knowledge, no one in the project worries about IntelliSense issues. I'm not certain we've had an explicit policy decision on this, but that's unsurprising given that IntelliSense is not required to build the project.

However, I would expect that the choice had been made in the past. If IntelliSense misbehavior are indeed "often" (as you said), and given that there aren't other IntelliSense complaints of header files in the code base (as far as I can tell), it is likely that a choice had been made in the past that IntelliSense errors should be avoided.

I think there aren't complaints about it because it doesn't have any impact other than putting up red squiggles where it shouldn't (or not having red squiggles where it should) in a text editor. I use the Visual Studio editor all day every day and do not find the IntelliSense issues to be anything more than a very minor distraction.

MSVC *does* support attribute namespaces in their implementation of __has_cpp_attribute (the compiler will parse and appropriately handle it); this is purely an IntelliSense thing, which is a cosmetic editor issue. The proposed changes disallow the macro from gracefully upgrading should Microsoft choose to implement any of the vendor attributes we use in some future version of MSVC. While I don't think that's a likely scenario for the attributes we currently use, I also don't think working around IntelliSense bugs is worth the effort unless there is some appreciable impact from the bug fix.

[haozhun]

Thank you for the explanation. I'm closing this off.

[aaron.ballman]

In D57196#1374495, @haozhun wrote:

Thank you for the explanation. I'm closing this off.

Thank you for looking into it! I'll file a report with Microsoft about the IntelliSense issue if you've not already done so.

include/llvm/Support/Compiler.h

 #endif
 
 #ifndef __has_attribute
 # define __has_attribute(x) 0
 #endif
 
 #ifndef __has_cpp_attribute
 # define __has_cpp_attribute(x) 0
+# define __has_cpp_attribute_qualified(x) 0
+#elif defined(_MSC_VER)
+// MSVC only allows simple identifier in feature-test macros,
+// and reports IntelliSense error otherwise.
+# define __has_cpp_attribute_qualified(x) 0
+#else
+# define __has_cpp_attribute_qualified __has_cpp_attribute
 #endif
 
 #ifndef __has_builtin
 # define __has_builtin(x) 0
 #endif
 
 /// \macro LLVM_GNUC_PREREQ
 /// Extend the default __GNUC_PREREQ even if glibc's features.h isn't

 
 /// LLVM_NODISCARD - Warn if a type or return value is discarded.
 #if __cplusplus > 201402L && __has_cpp_attribute(nodiscard)
 #define LLVM_NODISCARD [[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.
 #define LLVM_NODISCARD
-#elif __has_cpp_attribute(clang::warn_unused_result)
+#elif __has_cpp_attribute_qualified(clang::warn_unused_result)
 #define LLVM_NODISCARD [[clang::warn_unused_result]]
 #else
 #define LLVM_NODISCARD
 #endif
 
 // Indicate that a non-static, non-const C++ member function reinitializes
 // the entire object to a known state, independent of the previous state of
 // the object.

 #define LLVM_ATTRIBUTE_RETURNS_NOALIAS __declspec(restrict)
 #else
 #define LLVM_ATTRIBUTE_RETURNS_NOALIAS
 #endif
 
 /// LLVM_FALLTHROUGH - Mark fallthrough cases in switch statements.
 #if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
 #define LLVM_FALLTHROUGH [[fallthrough]]
-#elif __has_cpp_attribute(gnu::fallthrough)
+#elif __has_cpp_attribute_qualified(gnu::fallthrough)
 #define LLVM_FALLTHROUGH [[gnu::fallthrough]]
 #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.
 #define LLVM_FALLTHROUGH
-#elif __has_cpp_attribute(clang::fallthrough)
+#elif __has_cpp_attribute_qualified(clang::fallthrough)
 #define LLVM_FALLTHROUGH [[clang::fallthrough]]
 #else
 #define LLVM_FALLTHROUGH
 #endif
 
 /// LLVM_EXTENSION - Support compilers where we have a keyword to suppress
 /// pedantic diagnostics.
 #ifdef __GNUC__