[Clang][Sema] Make lambda in non-dependent context generate same analysis-based warnings as function[ template] #159364
Conversation
…nings as function[ template]
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Yanzuo Liu (zwuis) ChangesThis patch fixes following two issues:
auto L = [] {
return;
// previous: warning: left operand of comma operator has no effect
// expected: no warning, same as in functions
0, 0;
};
void f() {
auto L = [](auto) {
// previous: no warning
// expected: warning emitted same as in function templates
0, 0;
};
}Full diff: https://github.com/llvm/llvm-project/pull/159364.diff 6 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 518ed9e0f4b3e..0b68512d0c872 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -312,6 +312,9 @@ Improvements to Clang's diagnostics
properly being rejected when used at compile-time. It was not implemented
and caused assertion failures before (#GH158471).
+- Some reachability-analysis-based warnings in lambda expression which is in
+ non-templated context are emitted same as in function[ template].
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d017d1f829015..197b1eedc906d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -1166,7 +1166,7 @@ class Sema final : public SemaBase {
/// getCurFunctionOrMethodDecl - Return the Decl for the current ObjC method
/// or C function we're in, otherwise return null. If we're currently
/// in a 'block', this returns the containing context.
- NamedDecl *getCurFunctionOrMethodDecl() const;
+ NamedDecl *getCurFunctionOrMethodDecl(bool AllowLambda = false) const;
/// Warn if we're implicitly casting from a _Nullable pointer type to a
/// _Nonnull one.
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 39fa25f66f3b7..913a11bedc55d 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1656,8 +1656,8 @@ ObjCMethodDecl *Sema::getCurMethodDecl() {
return dyn_cast<ObjCMethodDecl>(DC);
}
-NamedDecl *Sema::getCurFunctionOrMethodDecl() const {
- DeclContext *DC = getFunctionLevelDeclContext();
+NamedDecl *Sema::getCurFunctionOrMethodDecl(bool AllowLambda) const {
+ DeclContext *DC = getFunctionLevelDeclContext(AllowLambda);
if (isa<ObjCMethodDecl>(DC) || isa<FunctionDecl>(DC))
return cast<NamedDecl>(DC);
return nullptr;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 73b16ae09e922..b6fdcfb646cb0 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20542,7 +20542,7 @@ void Sema::MarkDeclarationsReferencedInExpr(Expr *E,
/// namespace { auto *p = new double[3][false ? (1, 2) : 3]; }
bool Sema::DiagIfReachable(SourceLocation Loc, ArrayRef<const Stmt *> Stmts,
const PartialDiagnostic &PD) {
- if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
+ if (!Stmts.empty() && getCurFunctionOrMethodDecl(/*AllowLambda=*/true)) {
if (!FunctionScopes.empty())
FunctionScopes.back()->PossiblyUnreachableDiags.push_back(
sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp
index fbc2e7eb30676..2a6d0b5b4a9de 100644
--- a/clang/lib/Sema/SemaLambda.cpp
+++ b/clang/lib/Sema/SemaLambda.cpp
@@ -1972,6 +1972,10 @@ ExprResult Sema::ActOnLambdaExpr(SourceLocation StartLoc, Stmt *Body) {
if (LSI.CallOperator->hasAttr<SYCLKernelEntryPointAttr>())
SYCL().CheckSYCLEntryPointFunctionDecl(LSI.CallOperator);
+ // TODO: Find out if passing LSI.CallOperator->getDescribedFunctionTemplate()
+ // is necessary when it is a generic lambda. Are there any behaviour
+ // changes? `FunctionTemplateDecl` is always passed when handling simple
+ // function templates.
ActOnFinishFunctionBody(LSI.CallOperator, Body, /*IsInstantiation=*/false,
/*RetainFunctionScopeInfo=*/true);
@@ -2162,11 +2166,17 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc,
PopExpressionEvaluationContext();
- sema::AnalysisBasedWarnings::Policy WP =
- AnalysisWarnings.getPolicyInEffectAt(EndLoc);
// We cannot release LSI until we finish computing captures, which
// requires the scope to be popped.
- Sema::PoppedFunctionScopePtr _ = PopFunctionScopeInfo(&WP, LSI->CallOperator);
+ Sema::PoppedFunctionScopePtr _ = [&] {
+ if (LSI->CallOperator->getDescribedFunctionTemplate())
+ return PopFunctionScopeInfo(/*WP=*/nullptr,
+ TemplateOrNonTemplateCallOperatorDecl);
+
+ sema::AnalysisBasedWarnings::Policy WP =
+ AnalysisWarnings.getPolicyInEffectAt(EndLoc);
+ return PopFunctionScopeInfo(&WP, TemplateOrNonTemplateCallOperatorDecl);
+ }();
// True if the current capture has a used capture or default before it.
bool CurHasPreviousCapture = CaptureDefault != LCD_None;
diff --git a/clang/test/SemaCXX/warn-unused-value.cpp b/clang/test/SemaCXX/warn-unused-value.cpp
index 2a07a0324f3f0..7a2cbab275e0c 100644
--- a/clang/test/SemaCXX/warn-unused-value.cpp
+++ b/clang/test/SemaCXX/warn-unused-value.cpp
@@ -178,3 +178,39 @@ auto b() {
}
} // namespace test6
#endif
+
+// ensure lambda in non-dependent context generate same diagnostics as function[ template]
+namespace lambda_in_non_dependent_context {
+void f1() {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0;
+}
+template <typename T> void f2(T) {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+}
+auto L1 = [] {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0;
+};
+auto L2 = [](auto) {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+};
+void f() {
+ auto L1 = [] {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0;
+ };
+ auto L2 = [](auto) {
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ return;
+ 0, 0; // expected-warning {{left operand of comma operator has no effect}}
+ };
+}
+}
|
| // TODO: Find out if passing LSI.CallOperator->getDescribedFunctionTemplate() | ||
| // is necessary when it is a generic lambda. Are there any behaviour | ||
| // changes? `FunctionTemplateDecl` is always passed when handling simple | ||
| // function templates. |
There was a problem hiding this comment.
What do you mean? We don't call getDescribedFunctionTemplate here
clang/lib/Sema/SemaLambda.cpp
Outdated
| Sema::PoppedFunctionScopePtr _ = [&] { | ||
| if (LSI->CallOperator->getDescribedFunctionTemplate()) | ||
| return PopFunctionScopeInfo(/*WP=*/nullptr, | ||
| TemplateOrNonTemplateCallOperatorDecl); | ||
|
|
||
| sema::AnalysisBasedWarnings::Policy WP = | ||
| AnalysisWarnings.getPolicyInEffectAt(EndLoc); | ||
| return PopFunctionScopeInfo(&WP, TemplateOrNonTemplateCallOperatorDecl); | ||
| }(); |
There was a problem hiding this comment.
can we just
sema::AnalysisBasedWarnings::Policy *Policy = nullptr;
if (...)
Policy = &...;afaik Policy is a pretty small object to create on a stack anyway so we should avoid this nesting.
| void f1() { | ||
| 0, 0; // expected-warning {{left operand of comma operator has no effect}} | ||
| return; | ||
| 0, 0; |
There was a problem hiding this comment.
Why don't we have warnings for non-generic lambdas? Is that pre-existing issue?
There was a problem hiding this comment.
We have warnings controlled by -Wunreachable-code but it isn't enabled by default.
There was a problem hiding this comment.
But with your patch, we do have after-return warnings surfaced for generic-lambdas.
This makes us inconsistent.
Edit: I see we are already inconsistent with non-templates: https://gcc.godbolt.org/z/PsqYTrMEv
Does -Wunreachable-code change anything? The warnings are still not very great to me.
| // TODO: Find out if passing LSI.CallOperator->getDescribedFunctionTemplate() | ||
| // is better when it is a generic lambda. Are there any behaviour | ||
| // changes? `FunctionTemplateDecl` is always passed when handling simple | ||
| // function templates. |
There was a problem hiding this comment.
This comment is still here; I'd like to suggest we remove it because it doesn't look very clear: what behavior changes are we talking about/which function that Decl is passed to?
| Decl *TemplateOrNonTemplateCallOperatorDecl = | ||
| CallOperator->getDescribedFunctionTemplate() | ||
| ? CallOperator->getDescribedFunctionTemplate() | ||
| : cast<Decl>(CallOperator); |
There was a problem hiding this comment.
I think you can leave it as-is? That way you can
sema::AnalysisBasedWarnings::Policy *ActivePolicy = IsGenericLambda ? nullptr : &WP;
Sema::PoppedFunctionScopePtr _ =
PopFunctionScopeInfo(ActivePolicy, TemplateOrNonTemplateCallOperatorDecl);Though I have no idea why LSI->CallOperator doesn't work when it's a template.
| void f1() { | ||
| 0, 0; // expected-warning {{left operand of comma operator has no effect}} | ||
| return; | ||
| 0, 0; |
There was a problem hiding this comment.
But with your patch, we do have after-return warnings surfaced for generic-lambdas.
This makes us inconsistent.
Edit: I see we are already inconsistent with non-templates: https://gcc.godbolt.org/z/PsqYTrMEv
Does -Wunreachable-code change anything? The warnings are still not very great to me.
There was a problem hiding this comment.
With a second look, I don't think this is very specific to generic lambdas.
https://gcc.godbolt.org/z/hW84M6KsG
Maybe we should fix it on the analysis side, by changing how it checks the dependency of the surrounding context.
3 participants
This patch fixes following two issues:
Reachability analysis doesn't emit warnings if it's a dependent context. Non-null warning policy is passed to
Sema::PopFunctionScopeInfoand warnings disappear if it's a generic lambda. Global generic lambdas don't suffer from this issue because of the issue above.Before [Clang] Reapply "Only remove lambda scope after computing evaluation context" #154458,
Sema::PopFunctionScopeInfofor lambda is called bySema::ActOnFinishFunctionBody. This patch modifies arguments ofSema::PopFunctionScopeInfoonly because I didn't find behaviour changes of modifying arguments ofSema::ActOnFinishFunctionBodyin regression tests.