[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

[Richard Dzenis via Phabricator via cfe-commits]

RIscRIpt added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3609-3611
+.. Note:: To use a ``[[msvc::constexpr]]`` function in a constant context,
+   one may want to create a ``constexpr`` function-wrapper and invoke the
+   ``[[msvc::constexpr]]`` function within a ``[[msvc::constexpr]] return`` statement.
----------------
RIscRIpt wrote:
> As per latest comments, maybe I should remove this note?
Removed.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2817
+  "[[msvc::constexpr]] has effect only on function definitions and return statements">,
+  InGroup<IgnoredAttributes>;
 
----------------
> Given that the intended use case is for usage behind the scenes in the standard library, I don't think we should be changing our diagnostic output at all [there]

How about other places? Theoretically I could re-use existing warnings/errors with diagnostics saying `constexpr` instead of `[[msvc::constexpr]]`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5615-5627
+    if (canEvalMSConstexpr || isMSConstexpr) {
+      // Diagnose invalid usage of [[msvc::constexpr]] function
+      bool isConstructor = isa<CXXConstructorDecl>(Definition);
+      if (canEvalMSConstexpr) { // !isMSConstexpr
+        Info.FFDiag(CallLoc, diag::note_constexpr_invalid_function, 1)
+            << /*IsConstexpr*/ 0 << isConstructor << Definition;
+        Info.Note(Definition->getLocation(), diag::note_declared_at);
----------------
RIscRIpt wrote:
> rsmith wrote:
> > Given that the intended use case is for usage behind the scenes in the standard library, I don't think we should be changing our diagnostic output at all here. If the library, as an implementation detail, marks a non-`constexpr` function as `[[msvc::constexpr]]`, we shouldn't tell the user to add `[[msvc::constexpr]]` to their code to allow it to be called, after all, the annotation is an implementation detail of the MS standard library.
> Sounds fair, I will adjust this code if I'm unable to convince you with my new comments.
Removed, and made the check as a single `if` statement.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9591-9594
+  bool IsMSConstexpr = Info.CurrentCall->CanEvalMSConstexpr &&
+                       OperatorNew->hasAttr<MSConstexprAttr>();
   if (OperatorNew->isReservedGlobalPlacementOperator() &&
+      (Info.CurrentCall->isStdFunction() || IsMSConstexpr) && !E->isArray()) {
----------------
rsmith wrote:
> RIscRIpt wrote:
> > rsmith wrote:
> > > Do we really need this change? Was our existing check of whether the caller is in namespace `std` not sufficient for MS' standard library? I'd strongly prefer not to have a documented, user-visible attribute that gives permission to use placement new directly.
> > Yes, STL's `operator new` is defined in global namespace in [[ https://gist.github.com/RIscRIpt/9f0991f09f97eafc375fc73ea851a81b#file-vcruntime_new-h-L165 | vcruntime_new.h ]] (and all includes of this file are made from global namespace).
> The existing code is checking whether the caller of `operator new` (eg, `std::construct_at`) is in namespace `std`, not whether the `operator new` itself is. Does MSVC's `construct_at` need this? It might if it uses a placement new indirectly via a function in a different namespace, but it seems likely to me that it doesn't.
Yes, sorry, my bad. This change is not needed, unless we want to support this attribute in user-code, which I guess we don't want to.

Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134475