[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11188-11189
 
+  ConstexprSpecKind ConstexprKind = DetermineSpecialMemberConstexprKind(
+      Constexpr, ClassDecl->hasDefaultedConstevalDefaultCtor());
+
----------------
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



================
Comment at: clang/lib/Sema/SemaExpr.cpp:15071
+                    llvm::PointerIntPair<ConstantExpr *, 1> RHS) {
+      return LHS.getPointer()->getBeginLoc() < RHS.getPointer()->getBeginLoc();
+    }
----------------
rsmith wrote:
> `<` on `SourceLocation` doesn't really mean anything. It's not correct to use it here. Also, using source locations will not give you outer expressions before inner ones. Consider:
> 
> ```
> enum E {};
> consteval E f();
> consteval int operator+(E, int);
> void g() {
>   f() + 1;
> }
> ```
> 
>  Here, the begin location of both the call to `f()` and the call to `operator+` are in the same place: at the `f` token.
> 
> I don't think you need this sort at all: instead, you can process immediate invocations in `ImmediateInvocationCandidates` in reverse order: subexpressions must have been built before their enclosing expressions, so walking in reverse order will guarantee that you process an outer candidate before any inner ones.
the sort is used in combination with a lower_bounds in `ComplexRemove::TransformConstantExpr` to lookup in `ImmediateInvocationsCandidates` using binary search.
but i think your proposition is more efficient except in some very rare cases.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:15102
+                ExprCompType{});
+            if (It != IISet.end()) {
+              It->setInt(1); // Mark as deleted
----------------
rsmith wrote:
> We should look for an exact match here, not merely for something nearby. (Maybe: form a pointer set of immediate invocation pointer expressions, and remove things from the set when you encounter them in this walk; then when iterating over the immediate invocations, skip ones that have already been erased from the set.)
the issue i had with a set is that we can't remove element during iteration as removing element invalidate iterators.
but to get an exact match we can use the pointer as "key", they are unique.


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

https://reviews.llvm.org/D63960