ymandel added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:36
+/// FIXME: Consider replacing this with a model that is more aligned with C++
+/// value categories.
+enum class SkipPast {
----------------
I'm not sure that value categories are right here. The key is that skipping is
optional, unlike value categories which are fixed. I think the problem is just
that C++ references are weird (for lack of a technical term). A reference
variable exists simultaneously in two different categories -- a pointer and the
value it points to. That's what this is trying to capture.
So, I'd recommend just changing the name to reflect that. WDYT of something
like this?
```
/// Indicates how to treat references, if encountered -- as a reference or the
value referred to -- when retrieving
/// storage locations or values.
enum class RefTreatment {
/// Consider the reference itself.
AsSelf,
/// Consider the referent.
AsReferent,
};
```
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:111
+ /// value in the environment.
+ Value *getValue(const StorageLocation &Loc, SkipPast SP) const;
+
----------------
Why have a SkipPast argument here? I understand the concept for considering
var-decls - it corresponds to the weirdness of lvalue-refs themselves. But
StorageLocation is a lower-level (and simpler) concept, to which this doesn't
seem to map. I'm fine with resolving indirections, but what is the value of
only doing so conditionally?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:242-247
+ if (auto *Val =
+ dyn_cast_or_null<ReferenceValue>(getValue(Loc, SkipPast::None))) {
+ return Val->getPointeeLoc();
+ } else {
+ return Loc;
+ }
----------------
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:37
+ assert(S->getLHS() != nullptr);
+ const Expr *LHS = S->getLHS()->IgnoreParens();
+ assert(LHS != nullptr);
----------------
maybe comment as to why this is necessary? I'd have thought that CFG's
flattening of expressions would handle this (why, in general, we don't need to
look deep into expressions).
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:135
+
+ if (const Expr *InitExpr = D.getInit()) {
+ if (D.getType()->isReferenceType()) {
----------------
nit: might be easier to read if you invert this:
```
const Expr *InitExpr = D.getInit();
if (InitExpr == nullptr) return;
```
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:144-145
+ }
+ } else {
+ if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
+ Env.setValue(Loc, *InitExprVal);
----------------
nit: else-if (one line)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116596/new/
https://reviews.llvm.org/D116596
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
