RIscRIpt planned changes to this revision. RIscRIpt marked 2 inline comments as done. RIscRIpt added a comment.
> Does it prohibit the inverse? I think this documentation overall needs a > much better description of what the semantics are here, particularly anything > that you found in experimentation on the MS implementation. Do you mean, can `[[msvc::constexpr]]` function call `constexpr` and remain const-evaluated? Yes. From my experiments all Cody's points are valid, except for "An extended constexpr function can call other extended constexpr functions without a statement-level annotation". I tried to make doc notes as compact as possible. Recapping his points, which were confirmed by tests: 1. `[[msvc::constexpr]]` can be applied only to function definition or a return statement. 2. A function can be either `constexpr` or `consteval` or `[[msvc::constexpr]]`, but no combination of these. 3. `[[msvc::constexpr]]` function is treated the same way as `constexpr` function if it is evaluated in terms of `[[msvc::constexpr]] return` statement, otherwise it's a regular function. By function I mean anything function-like: functions, member-functions, constructors, destructors, operators. In D134475#4287513 <https://reviews.llvm.org/D134475#4287513>, @erichkeane wrote: > Still suspicious about this/whether it models the MSVC implementation > correctly, but no real implementation concerns. I REALLY want us to document > the attribute extensively however. Thanks to your concern I decided to test each point of `[dcl.constexpr]` : `9.2.6 The constexpr and consteval specifiers` in Compiler Explorer: https://godbolt.org/z/qj3fnK5aW I am going to update tests accordingly. Regarding on absolute matching MSVC implementation, I am not sure we will be able to achieve it with reasonable changes. I am skeptical, because I discovered the following test case: struct S2 { [[msvc::constexpr]] S2() {} [[msvc::constexpr]] bool value() { return true; } static constexpr bool check() { [[msvc::constexpr]] return S2{}.value(); } }; static_assert(S2::check()); It's a valid code for MSVC; nothing special https://godbolt.org/z/znnaonEhM However supporting this code seems to be difficult in Clang: 1. S2 fails checks of "literal type" in this callstack: 1. [[ https://github.com/llvm/llvm-project/blob/bc73ef0/clang/include/clang/AST/DeclCXX.h#L1419-L1445 | `clang::CXXRecordDecl::isLiteral` ]] 2. [[ https://github.com/llvm/llvm-project/blob/e98776a/clang/lib/AST/Type.cpp#L2746 | `clang::Type::isLiteralType` ]] 3. [[ https://github.com/llvm/llvm-project/blob/a4edc2c/clang/lib/AST/ExprConstant.cpp#L2317-L2320 | `CheckLiteralType` ]] 2. We have information about CanEvalMSConstexpr only in `CheckLiteralType`. So, currently, I don't see how we can reasonably check whether `S2` is a valid literal type in context of `[[msvc::constexpr]]`. Two obvious **ugly** solutions are: 1. Copy all checks from `clang::CXXRecordDecl::isLiteral` to `CheckLiteralType` - violates DRY; two places shall be maintained. 2. Propagate `CanEvalMSConstexpr` down to `clang::CXXRecordDecl::isLiteral`. I don't think it's reasonable for supporting rare vendor-specific attribute. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:3543 +definition or a return statement. It has no effect on function declarations. +This attribute permits constant evaluation of ``[[msvc::constexpr]]`` functions +in ``[[msvc::constexpr]] return`` statements inside ``constexpr`` or ---------------- erichkeane wrote: > Does it prohibit the inverse? I think this documentation overall needs a > much better description of what the semantics are here, particularly anything > that you found in experimentation on the MS implementation. > Does it prohibit the inverse? Do you mean, can `[[msvc::constexpr]]` function call `constexpr` and remain const-evaluated? Yes. I elaborated on semantics in non-inline comment. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7057-7058 +static void handleMSConstexprAttr(Sema &S, Decl *D, const ParsedAttr &AL) { + if (auto *FD = dyn_cast<FunctionDecl>(D)) + FD->setConstexprKind(ConstexprSpecKind::Constexpr); + D->addAttr(::new (S.Context) MSConstexprAttr(S.Context, AL)); ---------------- RIscRIpt wrote: > aaron.ballman wrote: > > We can use `cast` here because we know the declaration must have already > > been determined to be a function (from the subjects list in Attr.td). > > > > That said, I think there's more semantic checking we want to do here. For > > example, can you use this attribute on a virtual function? What about a > > function already marked `constexpr`? Even more scary, what about a function > > marked `consteval`? > > > > Also, I presume a `constexpr` function should be able to call an > > `[[msvc::constexpr]]` function and vice versa? > > We can use cast here because we know the declaration must have already been > > determined to be a function (from the subjects list in Attr.td). > > Indeed. Changed. > > > That said, I think there's more semantic checking we want to do here. For > > example, can you use this attribute on a virtual function? What about a > > function already marked constexpr? Even more scary, what about a function > > marked consteval? > > Good questions. By running `strings c1xx.dll` I was able to find only the > following diagnostic messages regarding `[[msvc::constexpr]]`: > ``` > [[msvc::constexpr]] may only be applied to statements and functions" > [[msvc::constexpr]] cannot be applied to a 'constexpr' or 'consteval' > function" > ``` > > I've made a similar check here and added relevant test cases in > `msvc-attrs-invalid.cpp`. But in order to make it possible, I had to change > `FunctionDecl::isConstexpr()`, please check new changes. > > //TODO//: I think, I'll need to read more about `constexpr` for functions in > standard (and LLVM code), to add relevant restrictions here. Added a check that `[[msvc::constexpr]]` function has no `constexpr` or `consteval` specifiers. A member function can be `[[msvc::constexpr]]` the same way as `constexpr`. Regarding `virtual` - MSVC does not complain about `[[msvc::constexpr]]`, but it fails during constant evaluation (in case of C++17 and earlier). We have two approaches to address `virtual` case: (1) don't add the attribute to the declaration and emit a warning, or (2) check for `virtual` and `opts.Cplusplus17` during constant evaluation. I am inclining to (2). 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
