xazax.hun 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 {
----------------
ymandel wrote:
> 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,
> };
> ```
Yeah, value categories are indeed fixed, I was thinking along the lines of
treating expressions as if there was an `LValueToRValue` or `RValueToLValue`
conversion. I think some of this problem might come from the ambiguity. E.g.,
when I want to query the analysis state about the value of `*p`, what do I
want? Do I want the value for `*p` as an lvalue or `*p` as an rvalue? I would
get a different answer in both cases. If I see `*p` in the source code it is
fixed but when I want to query the analysis state it is not.
On the other hand I see that `SkipPast` is used in the transfer functions, and
I am wondering if that is justified. According to the language rules, I can
never observe the actual value of the reference. I can modify the pointee,
(`ref = val`) or create a pointer to the same value (`&ref`), but I cannot
actually read or modify what is inside the reference. So I wonder if it might
be less error prone if there would be no way in the transfer functions to
observe the values of the references either.
But I do not have a strong feeling about any of this at this point so feel free
to leave everything as it is.
> So, I'd recommend just changing the name to reflect that.
I am not sure how clear `Referent` is. For a non-native speaker `Pointee` might
be clearer, although I do see how it can be confusing.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:95
+StorageLocation &Environment::createStorageLocation(const Expr &E) {
+ // Evaluated expressions are always assigned the same storage locations to
+ // ensure that the environment stabilizes across loop iterations. Storage
----------------
sgatev wrote:
> xazax.hun wrote:
> > What is the model for expressions that evaluate to different locations in
> > every loop iterations, e.g.:
> > ```
> > for(int *p = array; p != array + size; ++p)
> > *p; // The dereference expression
> > ```
> >
> > Here, having `*p` always evaluate to the same location is not correct, we'd
> > probably need a way to widen this.
> In the current model, the storage location for `p` is stable and the value
> that is assigned to it changes. So, in one iteration the value assigned to
> the storage location for `p` is `val1` which is a `PointerValue` that points
> to `loc1` and in another iteration the value assigned to the storage location
> for `p` is `val2` which is a `PointerValue` that points to `loc2`. The
> storage location for `*p` is also stable and the value that is assigned to it
> is a `ReferenceValue` that points to either `loc1` or `loc2`. I implemented
> this and added a test.
In this case, does it really make sense to materialize this mapping? If every
subexpression corresponds to a unique location, and always the same location,
we could use the subexpression directly as a location.
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
