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