rsmith added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:1033-1035
+ bool IsCausedByImmediateInvocation() const {
+ return ConstantExprBits.IsCausedByImmediateInvocation;
+ }
----------------
I'd remove the "CausedBy" here -- the `ConstantExpr` is our representation of
the immediate invocation.
Also, please start this function name with a lowercase `i` for local
consistency.
================
Comment at: clang/include/clang/Sema/Sema.h:1027
+ using IICandidate = llvm::PointerIntPair<ConstantExpr *, 1>;
+
----------------
`II` doesn't mean anything to a reader here; please expand the abbreviation.
================
Comment at: clang/include/clang/Sema/Sema.h:5438-5439
+ /// Wrap the expression in a ConstantExpr if it is a potention immediate
+ /// invocation
+ ExprResult CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl);
----------------
potention -> potential
Missing period at end of sentence.
================
Comment at: clang/lib/AST/ExprConstant.cpp:2012-2013
+ if (auto *VD = LVal.getLValueBase().dyn_cast<const ValueDecl *>())
+ if (auto *FD = dyn_cast<FunctionDecl>(VD))
+ if (FD->isConsteval()) {
----------------
Please add braces to these `if` statements (their controlled statement is too
long to omit the braces). As a general rule: if an inner construct needs
braces, put braces on the outer constructs too.
================
Comment at: clang/lib/AST/ExprConstant.cpp:13618
+ if (InPlace) {
+ LValue LVal;
+ if (!::EvaluateInPlace(Result.Val, Info, LVal, this) ||
----------------
This isn't sufficient: the evaluation process can refer back to the object
under construction (eg, via `this`), and we need an lvalue that actually names
the result in order for this to work.
I think you'll need to do something like creating a suitable object (perhaps a
`LifetimeExtendedTemporaryDecl`) in the caller to represent the result of the
immediate invocation, and passing that into here.
================
Comment at: clang/lib/Sema/Sema.cpp:164
isConstantEvaluatedOverride = false;
+ RebuildingImmediateInvocation = false;
----------------
Consider using a default member initializer instead.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:8835-8836
+ // specifier.
+ if (ConstexprKind == CSK_consteval)
+ if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD))
+ if (MD->getOverloadedOperator() == OO_New ||
----------------
Please add braces here.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:8836
+ if (ConstexprKind == CSK_consteval)
+ if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD))
+ if (MD->getOverloadedOperator() == OO_New ||
----------------
This rule also applies to non-member allocator functions. (Don't check for a
`CXXMethodDecl` here.)
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11188-11189
+ ConstexprSpecKind ConstexprKind = DetermineSpecialMemberConstexprKind(
+ Constexpr, ClassDecl->hasDefaultedConstevalDefaultCtor());
+
----------------
Tyker wrote:
> rsmith wrote:
> > I don't think this is necessary: implicit special members are never
> > `consteval`. (We use `DeclareImplicitDefaultConstructor` when there is no
> > user-declared default constructor, in which case there can't possibly be a
> > defaulted consteval default constructor.) I don't think you need any of the
> > `DetermineSpecialMemberConstexprKind` machinery, nor the tracking of
> > `DefaultedSpecialMemberIsConsteval` in `CXXRecordDecl`.
> all the code you mention is to fixe an issue i saw while working on consteval.
> the AST of
> ```
> struct A {
> consteval A() = default;
> consteval A(int) { }
> };
> ```
> is
> ```
> TranslationUnitDecl
> `-CXXRecordDecl <line:1:1, line:4:1> line:1:8 struct A definition
> |-DefinitionData pass_in_registers empty standard_layout trivially_copyable
> trivial literal has_user_declared_ctor has_constexpr_non_copy_move_ctor
> can_const_default_init
> | |-DefaultConstructor exists trivial constexpr defaulted_is_constexpr
> | |-CopyConstructor simple trivial has_const_param needs_implicit
> implicit_has_const_param
> | |-MoveConstructor exists simple trivial needs_implicit
> | |-CopyAssignment trivial has_const_param needs_implicit
> implicit_has_const_param
> | |-MoveAssignment exists simple trivial needs_implicit
> | `-Destructor simple irrelevant trivial needs_implicit
> |-CXXRecordDecl <col:1, col:8> col:8 implicit referenced struct A
> |-CXXConstructorDecl <line:2:5, col:27> col:15 constexpr A 'void ()'
> default trivial noexcept-unevaluated 0x55585ae25160
> `-CXXConstructorDecl <line:3:5, col:24> col:15 consteval A 'void (int)'
> |-ParmVarDecl <col:17> col:20 'int'
> `-CompoundStmt <col:22, col:24>
> ```
> [[ https://godbolt.org/z/oRx0Ss | godbolt ]]
> notice that `A::A()` is marked as constexpr instead of consteval.
> and `A::A(int)` is marked correctly.
>
> this is because consteval defaulted special members are not marked as
> consteval.
> those code changes are intended to fix that. with this patch both constructor
> are marked as consteval
>
This change is incorrect: an implicit special member is never `consteval`.
(Note that implicit special members are the ones the compiler declares for you,
not the ones you get from `= default`.) We don't need this, and as a
consequence we don't need `DefaultedSpecialMemberIsConsteval` on
`DefinitionData`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:15192
+ FD = cast<FunctionDecl>(Call->getCalleeDecl());
+ else if (auto *Call = dyn_cast<CXXMemberCallExpr>(InnerExpr))
+ FD = Call->getMethodDecl();
----------------
This is unreachable; a `CXXMemberCallExpr` is a `CallExpr` so this was already
handled above. Can you just delete this case?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:15197
+ else
+ llvm_unreachable("unhandeld decl kind");
+ assert(FD->isConsteval());
----------------
Typo unhandeld -> unhandled
================
Comment at: clang/lib/Sema/SemaExpr.cpp:15248
+ bool ReplacingOriginal() { return true; }
+ } Transfromer(SemaRef, Rec.ReferenceToConsteval,
+ Rec.ImmediateInvocationCandidates, It);
----------------
Transfromer -> Transformer
================
Comment at: clang/lib/Sema/TreeTransform.h:3451-3452
- if (auto *FE = dyn_cast<FullExpr>(Init))
- Init = FE->getSubExpr();
-
----------------
I'm surprised that we can remove this; I'd expect this to be necessary to
properly handle transforming cases such as
```
struct X { X(int); ~X(); int n; };
struct Y { explicit Y(int); };
template<typename T> void f() {
Y y(X(sizeof(sizeof(T))).n);
}
template void f<int>();
```
... where we need to strip off an `ExprWithCleanups` to find the syntactic
initializer inside it.
If you want to special-case this from `ComplexRemove`, to make sure you see all
`ConstantExpr`s, can you override `TransformInitializer` there instead of
changing it globally?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63960/new/
https://reviews.llvm.org/D63960
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
