-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[clang] Detect dangling assignment for "Container<Pointer>" case. #108205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesThis is a follow up of #107213, supporting the assignment case. With this patch, clang now diagnoses cases where a dangling
Fixes #100526 Full diff: https://github.com/llvm/llvm-project/pull/108205.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 59ccdf1e15cd81..43f0d6eb4f2edc 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -300,6 +300,8 @@ Improvements to Clang's diagnostics
- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object is constructed, e.g. ``std::vector<string_view> v = {std::string()};`` (#GH100526).
+- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object is assigned, e.g. ``v = {std::string()};`` (#GH100526).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index c8e703036c132c..6fc1d4d0aae259 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -982,7 +982,8 @@ static bool shouldRunGSLAssignmentAnalysis(const Sema &SemaRef,
diag::warn_dangling_lifetime_pointer_assignment, SourceLocation());
return (EnableGSLAssignmentWarnings &&
(isRecordWithAttr<PointerAttr>(Entity.LHS->getType()) ||
- isAssignmentOperatorLifetimeBound(Entity.AssignmentOperator)));
+ isAssignmentOperatorLifetimeBound(Entity.AssignmentOperator) ||
+ isContainerOfPointer(Entity.LHS->getType()->getAsRecordDecl())));
}
static void checkExprLifetimeImpl(Sema &SemaRef,
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 234e06f069074b..d744140800f595 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -601,17 +601,23 @@ void test() {
std::optional<std::string_view> o4 = std::optional<std::string_view>(s);
// FIXME: should work for assignment cases
- v1 = {std::string()};
- o1 = std::string();
+ v1 = {std::string()}; // expected-warning {{object backing the pointer}}
+ o1 = std::string(); // expected-warning {{object backing the pointer}}
// no warning on copying pointers.
std::vector<std::string_view> n1 = {std::string_view()};
+ n1 = {std::string_view()};
std::optional<std::string_view> n2 = {std::string_view()};
+ n2 = {std::string_view()};
std::optional<std::string_view> n3 = std::string_view();
+ n3 = std::string_view();
std::optional<std::string_view> n4 = std::make_optional(std::string_view());
+ n4 = std::make_optional(std::string_view());
const char* b = "";
std::optional<std::string_view> n5 = std::make_optional(b);
+ n5 = std::make_optional(b);
std::optional<std::string_view> n6 = std::make_optional("test");
+ n6 = std::make_optional("test");
}
std::vector<std::string_view> test2(int i) {
|
@@ -601,17 +601,23 @@ void test() { | |||
std::optional<std::string_view> o4 = std::optional<std::string_view>(s); | |||
|
|||
// FIXME: should work for assignment cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove fixme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
// no warning on copying pointers. | ||
std::vector<std::string_view> n1 = {std::string_view()}; | ||
n1 = {std::string_view()}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly related to this PR, but we could add some tests for more deeply nested containers like std::vector<std::vector<std::string_view>>
. Regardless if we gat warnings for those or not, documenting whether this case works in tests have some value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, added. Current we only support the first-level nested container.
041b369
to
ff2f955
Compare
This is a follow up of #107213, supporting the assignment case.
With this patch, clang now diagnoses cases where a dangling
container<pointer>
is assigned, e.g.Fixes #100526