[PATCH] D134475: Add C++11 attribute msvc::constexpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D134475#3829583 <https://reviews.llvm.org/D134475#3829583>, @RIscRIpt wrote:

>> TODO: I think, I'll need to read more about constexpr for functions in standard (and LLVM code), to add relevant restrictions here.
>
> In the standard I was able to find the following paragraphs about `constexpr`:
>
> 1. Neither constructor nor destructor can be constexpr if the class has some virtual base class; this holds for `[[msvc::constexpr]]` in MSVC 14.33
> 2. `constexpr` is not a part of function signature, so same non-constexpr function cannot be defined. The same holds for C++11 attributes, and `[[msvc::constexpr]]` in MSVC 14.33
>
> I will add two relevant tests in `msvc-attrs-invalid.cpp`

Good, thank you!

> Regarding LLVM code, I made the following observations:
>
> - `FunctionDecl::isConstexprSpecified()` is mostly used for printing code, in other words, to check that the function is `constexpr` (and not `consteval`).

Sort of. A templated function marked with `constexpr` remains a constexpr function even if it can't be executed at compile time, so `isConstexprSpecified()` is really telling you "did the user write `constexpr` on this declaration?" instead of "is this function really constexpr?" (http://eel.is/c++draft/dcl.constexpr#4)

> - `FunctionDecl::getConstexprKind()` is used by:
> - `ASTImporter.cpp` - seems that, it should take care of attributes independently from the `getConstexprKind()`
> - `ASTWriterDecl.cpp` - seems that, it should take care of attributes independently from the `getConstexprKind()`
> - `SemaDecl.cpp` / `SemaDeclCXX.cpp` - checks that re-declaration was not performed with different constexpr specifier. MSVC and clang with current implementation allows re-declaration with different attributes (there are tests with `f2-f5` in `msvc-attrs.cpp`)
> - `SemaDeclCXX` - enables C++ constexpr constructor inheritance - problem - my current implementation supports it, whereas MSVC doesn't:
>
>   // Check '[[msvc::constexpr]]' is not taken into account during constructor inheritance
>   struct s2 {
>       [[msvc::constexpr]] s2() {}
>       constexpr operator bool() { return false; };
>   };
>   
>   struct s3 : s2 {
>       constexpr operator bool() { return true; };
>   };
>   static_assert(s3()); // MSVC: C2131: expression did not evaluate to a constant
>   static_assert(s3()); // clang with my patch: OK

Hmmm, that's interesting! I wonder if that's intentional behavior for MSVC or a bug. I'm guessing there are no open issues on Microsoft's bug tracker because this isn't a documented feature? Might be worth filing one to see what they think though. (Might not be worth it either, see below.)

> - `SemaTemplate.cpp` - enables C++ template re-specialization with different constexpr specifier, not a problem, because we are allowed to do the same with attributes (don't we?)

Shake a magic 8-ball, unfortunately. There's a core issue open currently about template specializations and attributes: http://wg21.link/cwg2604

> - `SemaTemplateInstantiateDecl.cpp` (`TemplateDeclInstantiator`) - enables C++ template instantiation with same constexpr specifier
>
>   template<typename T>
>   struct s1 {
>       [[msvc::constexpr]] s1() {}
>       constexpr operator bool() { return true; };
>   };
>   
>   static_assert(s1<void>()); // MSVC: C2131: expression did not evaluate to a constant
>   static_assert(s1<void>()); // clang with my patch: OK

Can you do: `constexpr s1<void> s;` in MSVC or does that also fail? (e.g., is the issue with the constructor or with the conversion operator?)

> - `TreeTransform.h` - code is related to lambdas - out-of-scope of the current patch.
>
> Regarding `MSVC:C2131` I found one more problem:
>
>   [[msvc::constexpr]] int C() { return 0; }
>   constexpr int V = C(); // MSVC: C2131: expression did not evaluate to a constant
>   constexpr int W = C(); // clang with my patch: OK

What the heck, that's a surprise! I would have expected that to work the same in Clang and MSVC (accepted by both).

> I am starting to question my patch - does LLVM really need it, if
>
> 1. There's no documentation for `[[msvc::constexpr]]`
> 2. The trivial implementation makes it an alias, but the semantic behavior is different - we could fix that, but there are lots of things to check
>
> @aaron.ballman WDYT?
>
> P.S. I'd rather abandon it, than trying to match behavior of MSVC.

If you don't want to work on the feature, that's totally fine (you're under no obligation to finish this if it turns out to be more than you wanted to take on)! I think we're going to need to support this attribute at some point because `<vcruntime.h>` has `#define _MSVC_CONSTEXPR [[msvc::constexpr]]` and that macro is used by the STL headers. But the usage I am seeing there is even more interesting than what we've discovered so far. From `<memory>`:

  constexpr _Ty* operator()(_Ty* _Location, _Types&&... _Args) const
      noexcept(noexcept(::new (const_cast<void*>(static_cast<const volatile void*>(_Location)))
              _Ty(_STD forward<_Types>(_Args)...))) /* strengthened */ {
      // clang-format on
      _MSVC_CONSTEXPR return ::new (const_cast<void*>(static_cast<const volatile void*>(_Location)))
          _Ty(_STD forward<_Types>(_Args)...);
  }

It's using the attribute on a return statement, not on a declaration! So I suspect there's some more reverse engineering needed unless Microsoft wanted to document their extension (which might be the bug we want to file instead of "is it intentional that this works in this weird way?" bug reports).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134475