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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits