ilya-biryukov added a comment. In D148802#4283566 <https://reviews.llvm.org/D148802#4283566>, @erichkeane wrote:
> My one concern is that this is going to expose our incorrect instantiation of > lambdas further and more painfully (see > https://github.com/llvm/llvm-project/issues/58872). Else, I don't see > anything to be concerned about here. That's true, but it seems that as a result of this patch, Clang will start rejecting some valid C++ programs when lambdas are used in unevaluated contexts, but will stop (incorrectly) accepting a class of programs that are invalid according to C++ standard. I would err on the side of false negatives (correct program not compiled) instead of false positives (incorrect program complied). Otherwise, future changes that make Clang compliant can potentially require rewriting the code that relied on incorrect behavior. It's still unfortunate that we don't accept valid programs, but at least existing code will not need to be changed after GH58872 <https://github.com/llvm/llvm-project/issues/58872> is fixed. My perspective is based on the need to support a large codebase, in which codebase-wide refactorings are expensive, other might have a different opinion here. ================ Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:965 + case CodeSynthesisContext::LambdaExpressionSubstitution: + // FIXME: add a note for lambdas. break; ---------------- erichkeane wrote: > ilya-biryukov wrote: > > erichkeane wrote: > > > Would really like this note here, it shouldn't be too difficult, right? > > Ah, sorry, I added a comment here that I forgot to submit. The question is: > > could it be that we want to skip this note? > > > > I wanted to double-check if folks find this note useful. > > On one hand, this probably creates some noise as there will always be other > > notes that point into the location of a corresponding substitution location > > that contains the lambda. > > On the other hand, since the lambda is not an immediate context, this may > > give hints to users on why SFINAE does not apply. > > > > If you feel like the note is useful, I will follow up with an > > implementation. > I think it is useful for exactly the reason you mentioned: this is going to > be somewhat shocking behavior to most people, so explaining it better will be > helpful. Makes sense. Added a corresponding note. ================ Comment at: clang/test/SemaCXX/warn-unused-lambda-capture.cpp:192 void test_use_template() { - test_templated<int>(); // expected-note{{in instantiation of function template specialization 'test_templated<int>' requested here}} + test_templated<int>(); // expected-note 13{{in instantiation of function template specialization 'test_templated<int>' requested here}} } ---------------- shafik wrote: > Why the 12 extra notes here, I feel I am missing something but not sure what. > I see the increase in notes in other cases as well. I'm not entirely sure, but it seems there is some deduplication of notes that's happening when the stacks of code-synthesis-contexts for subsequent errors are the same. However, this patch introduces a different code synthesis context for lambda substitutions, forcing the notes to be added. In the new version of the patch, the added context actually shows up in the notes as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148802/new/ https://reviews.llvm.org/D148802 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
