This is an archive of the discontinued LLVM Phabricator instance.

Differential D155202

[clang][dataflow] Simplify implementation of `transferStdForwardCall()` in optional check.
ClosedPublic

Authored by mboehme on Jul 13 2023, 7:23 AM.

Details

Reviewers
NoQ
ymandel
gribozavr2
xazax.hun
Commits
rG243a79ca01f8: [clang][dataflow] Simplify implementation of `transferStdForwardCall()` in…
Summary

The argument and return value of std::forward is always a reference, so we can simply forward the storage location.

Depends On D155075

Diff Detail

Event Timeline

mboehme created this revision.Jul 13 2023, 7:23 AM
Herald added a reviewer: NoQ. · View Herald TranscriptJul 13 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: martong, xazax.hun. · View Herald Transcript
mboehme requested review of this revision.Jul 13 2023, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 7:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mboehme added a child revision: D155204: [clang][dataflow] Add `refreshStructValue()`..Jul 13 2023, 7:24 AM
mboehme added a reviewer: ymandel.Jul 13 2023, 7:24 AM
Harbormaster completed remote builds in B245113: Diff 540023.Jul 13 2023, 7:24 AM
mboehme edited the summary of this revision. (Show Details)Jul 13 2023, 7:25 AM
gribozavr2 accepted this revision.Jul 13 2023, 7:28 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
683

Should we add an assert that it is not a value?

This revision is now accepted and ready to land.Jul 13 2023, 7:28 AM
ymandel accepted this revision.Jul 13 2023, 8:58 AM
mboehme marked an inline comment as done.Jul 14 2023, 12:05 AM
mboehme added inline comments.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
683

getStorageLocationStrict already contains such an assertion.

xazax.hun accepted this revision.Jul 15 2023, 7:42 AM
This revision was landed with ongoing or failed builds.Jul 17 2023, 12:26 AM
Closed by commit rG243a79ca01f8: [clang][dataflow] Simplify implementation of `transferStdForwardCall()` in… (authored by mboehme). · Explain Why
This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.
mboehme added a commit: rG243a79ca01f8: [clang][dataflow] Simplify implementation of `transferStdForwardCall()` in….

Revision Contents

PathSize
clang/
lib/
Analysis/
FlowSensitive/
Models/
21 lines

Diff 540023

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

Show First 20 Lines • Show All 674 Lines • ▼ Show 20 Lines auto *Arg1Loc = cast_or_null<AggregateStorageLocation>(
State.Env.getStorageLocationStrict(*E->getArg(1))); State.Env.getStorageLocationStrict(*E->getArg(1)));
transferSwap(Arg0Loc, Arg1Loc, State.Env); transferSwap(Arg0Loc, Arg1Loc, State.Env);
} }
void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &, void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) { LatticeTransferState &State) {
assert(E->getNumArgs() == 1); assert(E->getNumArgs() == 1);
StorageLocation *LocRet = State.Env.getStorageLocation(*E, SkipPast::None); if (auto *Loc = State.Env.getStorageLocationStrict(*E->getArg(0)))
gribozavr2Unsubmitted
Done
ReplyInline Actions

Should we add an assert that it is not a value?

gribozavr2: Should we add an assert that it is not a value?
mboehmeAuthorUnsubmitted
Done
ReplyInline Actions

getStorageLocationStrict already contains such an assertion.

mboehme: `getStorageLocationStrict` already contains such an assertion.
if (LocRet != nullptr) State.Env.setStorageLocationStrict(*E, *Loc);
return;
StorageLocation *LocArg =
State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
if (LocArg == nullptr)
return;
Value *ValArg = State.Env.getValue(*LocArg);
if (ValArg == nullptr)
ValArg = &createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env);
// Create a new storage location
LocRet = &State.Env.createStorageLocation(*E);
State.Env.setStorageLocation(*E, *LocRet);
State.Env.setValue(*LocRet, *ValArg);
} }
BoolValue &evaluateEquality(Environment &Env, BoolValue &EqVal, BoolValue &LHS, BoolValue &evaluateEquality(Environment &Env, BoolValue &EqVal, BoolValue &LHS,
BoolValue &RHS) { BoolValue &RHS) {
// Logically, an optional<T> object is composed of two values - a `has_value` // Logically, an optional<T> object is composed of two values - a `has_value`
// bit and a value of type T. Equality of optional objects compares both // bit and a value of type T. Equality of optional objects compares both
// values. Therefore, merely comparing the `has_value` bits isn't sufficient: // values. Therefore, merely comparing the `has_value` bits isn't sufficient:
// when two optional objects are engaged, the equality of their respective // when two optional objects are engaged, the equality of their respective
▲ Show 20 Lines • Show All 355 Lines • Show Last 20 Lines