[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. In D119544#3527556 , @erichkeane wrote: > In D119544#3526810 , @ChuanqiXu > wrote: > >> In D119544#3494281 , @erichkeane >> wrote: >> >>>

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3526810 , @ChuanqiXu wrote: > In D119544#3494281 , @erichkeane > wrote: > >> Updated to include the fix for the libcxx issue, which was that we weren't >> properly

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. In D119544#3494281 , @erichkeane wrote: > Updated to include the fix for the libcxx issue, which was that we weren't > properly differentiating between 'friend' functions based on concepts. I'll > likely be submitting this

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. Herald added a reviewer: dang. @rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is supposed to fix the friend function issues: > A non-template friend declaration with a requires-clause shall be a > definition. > A friend function template

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. I ended up having to revert again after my attempt this morning, this time due to some crash compiling a libc++ example. I haven't been able to repro it yet, but I'm hopeful that one of follow-up buildbots will give me a better reproducer command line. That said,

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 427359. erichkeane added a comment. Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. I was able to find a reproduction of the problem: template struct is_same { static constexpr bool value = false; }; template struct is_same { static constexpr bool value = false; }; template concept same_as = is_same::value; template

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3486573 , @gulfem wrote: >> Then pipe that to a file (note the -E I added at the end). You should get a >> file that looks like some slightly-wonky C++ code. > > I got the following output after running it via

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. > Then pipe that to a file (note the -E I added at the end). You should get a > file that looks like some slightly-wonky C++ code. I got the following output after running it via `-E`.

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3486385 , @gulfem wrote: > In D119544#3486241 , @erichkeane > wrote: > >> Ah shucks... Thanks for the heads up. Is there any chance to get you to get >> me a

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. In D119544#3486241 , @erichkeane wrote: > Ah shucks... Thanks for the heads up. Is there any chance to get you to get > me a pre-processed version of this failure to play with? I've not had luck > compiling/running libc++

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3486227 , @gulfem wrote: > We started seeing several test failures after this commit: > https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview > > One example

[PATCH] D119544: Deferred Concept Instantiation Implementation

gulfem added a comment. We started seeing several test failures after this commit: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview One example is `nothrow_forward_range.compile.pass.cpp`. Script: -- : 'COMPILED WITH';

[PATCH] D119544: Deferred Concept Instantiation Implementation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4b6c2cd647e9: Deferred Concept Instantiation Implementation (authored by erichkeane). Herald added a project: clang. Repository: rG LLVM Github

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 426060. erichkeane marked 3 inline comments as done. erichkeane added a comment. Make all the changes that @ChuanqiXu suggested. Thank you again so much for your help during this! I intend to commit this my Monday-AM unless someone comments differently.

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane marked 11 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:70 +assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?"); +assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?"); +

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM to me basically. Only comments for style left. Comment at: clang/lib/Sema/SemaConcept.cpp:70 +assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3477316 , @erichkeane wrote: > Correct the caching behavior to make the FriendFunc example work. > > I THINK this is ready to review! I'd like to do the lambda examples in a > followup patch as I believe that is

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 425516. erichkeane added a comment. Correct the caching behavior to make the FriendFunc example work. I THINK this is ready to review! I'd like to do the lambda examples in a followup patch as I believe that is a pre-existing issue, and this patch has

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. OK, 1 issue left (see `FriendFunc`). Comment at: clang/test/SemaTemplate/concepts.cpp:436 + SingleDepthReferencesTopLambda(v); + // TODO: This should error on constraint failure! (Lambda!) + SingleDepthReferencesTopLambda(will_fail);

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 425275. erichkeane added a comment. Update the concepts-test, remove some extra commented out code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:507 + + ContextRAII SavedContext{*this, const_cast(FD)}; + LocalInstantiationScope Scope(*this, true); erichkeane wrote: > This line was the one that fixed the variable lookup in

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:507 + + ContextRAII SavedContext{*this, const_cast(FD)}; + LocalInstantiationScope Scope(*this, true); This line was the one that fixed the variable lookup in non-templates. CHANGES

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 425224. erichkeane added a comment. Now crashes a few times in concepts.cpp, but sets up the instantiation-scope for functions such that local variable references work for them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. I found Comment at: clang/test/SemaTemplate/concepts.cpp:386-388 + []() +requires(constraint) + {}(); erichkeane wrote: > ChuanqiXu wrote: > > We might need more negative tests. > > Now it would pass even if I write: > > ``` >

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2431-2439 + //if (TrailingRequiresClause) { + // Sema::CXXThisScopeRAII ThisScope(SemaRef, Record, D->getMethodQualifiers()); + // ExprResult Rebuilt = + //

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. In D119544#3472537 , @erichkeane wrote: > Alright, a touch more: It looks like the looping through the `CurContext` at > line 6084 finds the wrong thing in evaluating this `CXXMethodDecl`! The > "broken" version has its

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. Alright, a touch more: It looks like the looping through the `CurContext` at line 6084 finds the wrong thing in evaluating this `CXXMethodDecl`! The "broken" version has its CurContext as `bar` in the example above, the "working" versions have `foo` or `foo2`.

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. Here is the example I'm playing with: template constexpr bool constraint = true; #define BROKE_REQUIRES 1 #define WORKING_REQUIRES 2 #define WORKING_BODY 3 #define TEST BROKE_REQUIRES //#define TEST WORKING_REQUIRES //#define TEST WORKING_BODY

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 424963. erichkeane added a comment. This revision is now accepted and ready to land. Here is my current 'WIP' patch, with only `ChecksMemberVar::foo` failing. I noticed that the problem so far starts in `TransformMemberExpr`, which calls `TransformDecl`

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts.cpp:391 + + CausesFriendConstraint CFC; + FriendFunc(CFC, 1); ChuanqiXu wrote: > erichkeane wrote: > > erichkeane wrote: > > > A bunch of the tests below this all fail. > > See

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added inline comments. Comment at: clang/test/SemaTemplate/concepts.cpp:391 + + CausesFriendConstraint CFC; + FriendFunc(CFC, 1); erichkeane wrote: > erichkeane wrote: > > A bunch of the tests below this all fail. > See these two tests, which fail by

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2329 + // constraint, so we should just create a copy of the previous one. + // TODO: ERICH: Should this be RebuildExprInCurrentInstantiation

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 424241. erichkeane added a comment. This revision is now accepted and ready to land. Found that the recursive var-decl collection was incorrect, since all the values were already in the parent scopes! So I ended up being able to fix MOST of the problems

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts.cpp:391 + + CausesFriendConstraint CFC; + FriendFunc(CFC, 1); A bunch of the tests below this all fail. CHANGES SINCE LAST ACTION

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 424183. erichkeane added a comment. This revision is now accepted and ready to land. Added the tests that still fail to 'concepts.cpp', I still need to figure those out. However, I switched our 'skipping of instantiation' over to use

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13002 + ExprResult NewTrailingRequiresClause = + E->getCallOperator()->getTrailingRequiresClause(); erichkeane wrote: > erichkeane wrote: > > cor3ntin wrote: > > > That doesn't

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13002 + ExprResult NewTrailingRequiresClause = + E->getCallOperator()->getTrailingRequiresClause(); erichkeane wrote: > cor3ntin wrote: > > That doesn't look right. > > At best

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:496 +llvm::Optional +Sema::SetupConstraintCheckingTemplateArgumentsAndScope( +FunctionDecl *FD, llvm::Optional> TemplateArgs, erichkeane wrote: > Ugh... it looks like all of this

[PATCH] D119544: Deferred Concept Instantiation Implementation

cor3ntin added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13002 + ExprResult NewTrailingRequiresClause = + E->getCallOperator()->getTrailingRequiresClause(); That doesn't look right. At best if you don't transform the trailing return

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added a comment. If anyone can help here, it would be vastly appreciated... I'm simply out of ideas on how to make this work. Comment at: clang/lib/Sema/SemaConcept.cpp:496 +llvm::Optional

[PATCH] D119544: Deferred Concept Instantiation Implementation

cor3ntin added a comment. Oh no. Let me know if i can help in any way CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. Well, crud. @cor3ntin broke me again :) I went to rebase to get pre-commit to run again, and now the problem is the lambda captures. Taking a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 423901. erichkeane marked 6 inline comments as done. erichkeane added a comment. Thanks for the review @ChuanqiXu ! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane marked 10 inline comments as done. erichkeane added inline comments. Comment at: clang/include/clang/AST/Decl.h:1891 +TK_DependentFunctionTemplateSpecialization, +// A Dependent function that itself is not a function. +TK_DependentNonTemplate

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/Decl.h:1891 +TK_DependentFunctionTemplateSpecialization, +// A Dependent function that itself is not a function. +TK_DependentNonTemplate hmmm, what does this literally mean? In my

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 423713. erichkeane added a comment. Herald added a subscriber: martong. Herald added a reviewer: shafik. Fixed the issue that Corentin's test came up with, added a number of others. The problem is that we didn't properly 'collect' the parameters in cases

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. FWIW, I am about 95% sure I have a hold on this, plus a bunch of other cases I came up with. I likely won't get a patch up for review today (in the next hour!) unless something miraculous happens, but hopefully I'll have something tomorrow for folks to take another

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3459169 , @tahonermann wrote: >> I wouldn't think so either? In this case the problem is that 'u' is not in >> the re-manufactured scope, I think there is a bit of work to make sure that >> lambdas ALSO get the

[PATCH] D119544: Deferred Concept Instantiation Implementation

tahonermann added a comment. > I wouldn't think so either? In this case the problem is that 'u' is not in > the re-manufactured scope, I think there is a bit of work to make sure that > lambdas ALSO get the scope of their containing function, if they are in a > functiondecl. I wouldn't expect

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3459045 , @tahonermann wrote: >> This is a case where the function is a template instantiation but does NOT >> have a primary template, so I have to figure out what THAT means/what I >> should be using instead. >

[PATCH] D119544: Deferred Concept Instantiation Implementation

tahonermann added a comment. > This is a case where the function is a template instantiation but does NOT > have a primary template, so I have to figure out what THAT means/what I > should be using instead. I think that is not supposed to be possible. For example,

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3458966 , @cor3ntin wrote: > In D119544#3458848 , @erichkeane > wrote: > >> I went to commit this, and found that a recently lit test now fails with a >> crash during

[PATCH] D119544: Deferred Concept Instantiation Implementation

cor3ntin added a comment. In D119544#3458848 , @erichkeane wrote: > I went to commit this, and found that a recently lit test now fails with a > crash during constraint instantiation! I'll be looking into that. The > example reduces to: > >

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. I went to commit this, and found that a recently lit test now fails with a crash during constraint instantiation! I'll be looking into that. The example reduces to: template constexpr bool constraint = true; template void dependent(U&& u) { []()

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 421564. erichkeane marked an inline comment as done. erichkeane added a comment. rebase for CI + comment change requested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files:

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM basically. Please wait for 1~2 weeks to land this in case there are other comments. Comment at: clang/lib/Sema/SemaConcept.cpp:496-497 + + // Attn Reviewers: we

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 421186. erichkeane marked 2 inline comments as done. erichkeane added a comment. Make suggestions from @ChuanqiXu CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane marked 8 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:4705 CheckConstraintSatisfaction( - NamedConcept, {NamedConcept->getConstraintExpr()}, Converted, + NamedConcept,

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. I've read the whole patch. It looks good to me roughly except some style issues and the 2 places you marked as ` Attn Reviewers`. Let's try to fix them one by one. Comment at: clang/lib/Sema/SemaTemplate.cpp:4705 CheckConstraintSatisfaction(

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61 const NamedDecl *D, const TemplateArgumentList *Innermost, -bool RelativeToPrimary, const FunctionDecl *Pattern) { +bool RelativeToPrimary, const FunctionDecl *Pattern,

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:489 + // Attn Reviewers: we need to do this for the function constraints for + // comparison of constraints to work, but do we also need to do it for + // CheckInstantiatedFunctionConstraints? That

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:489 + // Attn Reviewers: we need to do this for the function constraints for + // comparison of constraints to work, but do we also need to do it for + // CheckInstantiatedFunctionConstraints? That

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 419750. erichkeane added a comment. Do a rebase, only conflict was with ReleaseNotes.rst. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. BTW, it looks like the patch needs to rebase with main so that other people could play it self if interested. Comment at: clang/lib/Sema/SemaConcept.cpp:489 + // Attn Reviewers: we need to do this for the function constraints for + // comparison

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/include/clang/Sema/Sema.h:7011 + + bool CheckConstraintSatisfaction( + const NamedDecl *Template, ArrayRef ConstraintExprs, ChuanqiXu wrote: > I think this one need comment too. What's the difference with

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 419463. erichkeane marked 5 inline comments as done. erichkeane added a comment. Respond to @ChuanqiXu and fix a few comments he suggested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files:

[PATCH] D119544: Deferred Concept Instantiation Implementation

ChuanqiXu added a comment. I am interested in this one. But it is absolutely not easy to understand... Comment at: clang/include/clang/Sema/Sema.h:6991-6993 /// \param TemplateArgs the list of template arguments to substitute into the - /// constraint expression. + ///

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 418235. erichkeane added a comment. Apply clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Sema/Sema.h

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 417948. erichkeane added reviewers: clang-language-wg, aaron.ballman, saar.raz. erichkeane added a subscriber: cfe-commits. erichkeane added a comment. Added release notes. I tossed every ranges-based example I could find on google at this and I saw only