Skip to content

Fix FP#11508#4774

Merged
orbitcowboy merged 7 commits intocppcheck-opensource:mainfrom
Ashimaru:viewLifetime
Feb 18, 2023
Merged

Fix FP#11508#4774
orbitcowboy merged 7 commits intocppcheck-opensource:mainfrom
Ashimaru:viewLifetime

Conversation

@Ashimaru
Copy link
Copy Markdown
Contributor

@Ashimaru Ashimaru commented Feb 6, 2023

My current understanding is that views are treated as references - there is an issue that for references each of them has its own lifetime but view always uses lifetime of container it is 'viewing'. Lifetime of variable is done by comparing end of scopes where function is declared at and where it is used. Since those are 2 separate functions and not lambda FP is observed

@orbitcowboy orbitcowboy marked this pull request as ready for review February 6, 2023 17:02
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Feb 7, 2023

please check the CI failure. Seems there is assertion failure in the unit test.

@Ashimaru
Copy link
Copy Markdown
Contributor Author

Ashimaru commented Feb 8, 2023

yeah, this is still work in progress - right now I am still trying to figure out how views should be propagated - as refs or differently. Thats why it was marked as work in progress but seems like @orbitcowboy changed it

@Ashimaru Ashimaru marked this pull request as draft February 8, 2023 10:44
@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Feb 8, 2023

The problem is not how we propagate the views. Instead the checker should use different strategy when the lifetime is from a subfunction.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Feb 8, 2023

Ah understood. Sorry.

@Ashimaru
Copy link
Copy Markdown
Contributor Author

Ashimaru commented Feb 9, 2023

No problem :) @pfultz2 thanks for suggestion, I will work more on it in the weekend

Comment thread lib/valueflow.cpp Outdated
else
{
master.lifetimeScope = ValueFlow::Value::LifetimeScope::Local;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope should be Local here. We only set SubFunction scope when the lifetime is passed from another function, like in valueFlowSubFunction. This function only infers lifetimes locally.

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think the LifetimeScope needs to be changed. Instead we should only use local lifetimes when handling the indirection here(it should be v.isLocalLifetimeValue()):

https://github.com/danmar/cppcheck/blob/26bb55154f316b4e5bb3d7f20d8804b48fab8e16/lib/valueflow.cpp#L4819

Similar to the loop above when a dereference happens.

"}\n"
"void f(std::string_view text) {"
"g(text.data());\n"
"}\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases should be indented.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm my reasoning was that if value has subfunction lifetime then any pointer/iterator to said in this scope should also has same lifetime. I will adjust it, thanks.

@Ashimaru Ashimaru marked this pull request as ready for review February 15, 2023 20:05
@orbitcowboy orbitcowboy merged commit 9b62caf into cppcheck-opensource:main Feb 18, 2023
@Ashimaru Ashimaru deleted the viewLifetime branch February 18, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants