This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Model the behavior of non-standard optional constructors
ClosedPublic

Authored by sgatev on Mar 14 2022, 7:55 AM.

Details

Summary

Model nullopt, inplace, value, and conversion constructors.

Diff Detail

Event Timeline

sgatev created this revision.Mar 14 2022, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 7:55 AM
sgatev requested review of this revision.Mar 14 2022, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 7:55 AM
ymandel added inline comments.Mar 14 2022, 9:00 AM
clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h
42

This shouldn't be needed, since we now pass the MatchFinder::MatchResult in Action, which includes the ASTContext.

xazax.hun added inline comments.Mar 14 2022, 9:13 AM
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
55

Did you consider hasAnyName? Or would that not work in this context?

75

What is a non-standard constructor? This might be a well-known term I'm not aware of. If that is not the case I'd prefer a comment or a more descriptive name.

263

Do we need this + to force conversion to function pointer? Is it possible to fix CaseOf so it works with lambdas or is that not desirable?

ymandel added inline comments.Mar 14 2022, 11:12 AM
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
110

This could get expensive. Maybe add a FIXME to optimize this by avoiding getQualifiedNameAsString?

264

I realize this is consistent with the previous version, but just noticed this issue. By using assignOptionalValue for emplace (and reset), does that break the following case?

optional<int> opt;
if (p) opt.emplace(3);
someCode();
if (p) use(*opt);
sgatev updated this revision to Diff 415177.Mar 14 2022, 11:48 AM
sgatev marked 2 inline comments as done.

Address reviewers' comments.

sgatev updated this revision to Diff 415178.Mar 14 2022, 11:50 AM
sgatev marked 2 inline comments as done.

Add a FIXME.

sgatev marked 2 inline comments as done.Mar 14 2022, 12:03 PM
sgatev added inline comments.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
264

This isn't supported yet. Still, I don't think assignOptionalValue breaks it because this seems more related to the way information from different branches is joined than the way values are initialized. I'll add this case (and others) in a following patch that adds support for joining.

xazax.hun accepted this revision.Mar 14 2022, 12:06 PM
This revision is now accepted and ready to land.Mar 14 2022, 12:06 PM
xazax.hun added inline comments.Mar 14 2022, 12:08 PM
clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h
150

Nit: looks like we need to repeat the action type. Should we restore the using above?

xazax.hun added inline comments.Mar 14 2022, 12:10 PM
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
120

Nit: some of these functions are static and some not. Should we have one big anonymous namespace instead?

ymandel accepted this revision.Mar 14 2022, 12:36 PM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
264

Sounds good. I think we can discuss further in the next patch as needed.

sgatev updated this revision to Diff 415349.Mar 15 2022, 12:56 AM
sgatev marked 3 inline comments as done.

Address reviewers' comments.

clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h
150

It's not the same. The one above is a template - it accepts an arbitrary Node instead of a Stmt.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
120

We already have an anonymous namespace so I'll make the functions inside non-static.

This revision was landed with ongoing or failed builds.Mar 15 2022, 1:13 AM
This revision was automatically updated to reflect the committed changes.