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

[Richard Dzenis via Phabricator via cfe-commits]

RIscRIpt marked an inline comment as done.
RIscRIpt added a comment.

Thank you for your quick response.

> Given that the intended use case is for usage behind the scenes in the standard library...



> I'd strongly prefer not to have a documented, user-visible attribute that gives permission to use placement new directly.

I understand your concerns, and I cannot disagree. My reasoning behind the current implementation is to allow clang with `-fms-extensions` to parse and compile the same code with the `[[msvc::constexpr]]` attribute as MSVC can. Given that any user of the MSVC compiler can use this attribute and compile their code with it, it seems to me that clang-cl should also have this capability. However, we don't have any guarantees that Microsoft won't modify anything related to this attribute. I am not aware of the policies the Clang project has in place for such cases. Regardless, I will adjust the patch to comply as necessary.

TODO for my next patch: pre-define `_MSVC_CONSTEXPR_ATTRIBUTE`



================
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.
----------------
As per latest comments, maybe I should remove this note?


================
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);
----------------
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.


================
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:
> 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134475