On Tue, Oct 21, 2008 at 12:32 PM, Ted Kremenek <[EMAIL PROTECTED]> wrote:
> > On Oct 20, 2008, at 3:31 AM, Zhongxing Xu wrote: > > Some more comments: >> >> int a[10]; >> int (*p)[10]; >> >> Per C99 6.3.2p3, 'a' and '*p' is not lvalue when they are placed in a[1] >> or (*p)[1]. So in fact we cannot do lvalue evaluation of them. >> > > I'm looking at the section, and I'm not seeing the actual clause that your > are referencing. Is there a particular sentence or paragraph you are > referring to? I think this is the phrase: > > "Except when it is the operand of the sizeof operator or the unary & > operator, or is a string literal used to initialize an array, an expression > that has type ''array of type'' is converted to an expression with type > ''pointer to type'' that points to the initial element of the array object > and is not an lvalue." I think I made a mistake when understanding the last "and is not an lvalue". It should refer to the expression after conversion. > > > This conversion is handled by the ImplicitCastExpr. It seems fairly > straightforward to me to have VisitCast() call VisitLValue() on its child > expression when doing an array to pointer conversion. > > But this is inconsistent with the usual case in VisitDeclRefExpr and >> VisitUnaryExpr. But 'a' and '*p' do have lvalue sometimes. >> > > Perhaps, but arrays really are a special case, both in the language syntax > and the C type system. It's not surprising to me that we need to handle > them specifically. > > And special handling of these non-lvalue cases in the GRExprEngine is >> ugly. >> > > I agree it isn't pleasant, but the ugliness is highly localized. I believe > it's much better to put this ugliness in one place (the core analysis > engine) rather than require all implementations of Store to get it right. > GRExprEngine already takes care of lots of details like this. Agree. > > On the other hand, a simple trick in the Store will do us the favor. If we >> are passed loc::MemRegionVal(some array's region) in the store, we do not go >> through the binding, but simply return loc::MemRegionVal(some array's >> region) as the loaded value. Then we can do the usual thing for all cases in >> GRExprEngine's Visitors: get the lvalue of the Decl, ask Store what its >> rvalue is. >> > > Understood. Three comments. > > First, having the Store do this means that *every* implementation of Store > will have to specially implement this corner case. By doing it outside of > Store (i.e., in GRExprEngine), we might need to work hard to get it right, > but once we do it will be right for all implementations of Store. It's not > an obvious thing for a Store to do, which means people will likely get it > wrong. > > Second, after our long thread a week ago, my feeling is that the Store has > no notion of lvalues or rvalues, only locations and the values that are > binded to those locations. In this regards, I believe that GetSVal() should > only return the value that is bound to a region. The implementation of > GetSVal() should not have to guess of whether it is called to return an > lvalue or an rvalue given the type of a region. That just doesn't seem very > clean to me, and violates the separation of concerns between handling all > the nastiness of C's syntax in GRExprEngine and handling the semantics of > value bindings in Store. > > Third, what regions a Store returns to callers (via getLValueXXX) and the > meaning of those regions is entirely up to the Store. To me it makes no > sense to call GetSval() on a VarRegion for an array variable because the > operation of "loading from an array" doesn't make sense, i.e., there is no C > syntax that allows one to "load array" (or "get binding"); we can load an > element of an array, but not the array itself. The only way for GetSVal() to > make sense in this context is for it to return an lvalue, but that really > isn't the binding. Perhaps you have a different interpretation of > GetSVal(), but to me just keeping it to mean "fetch the value bound to this > location" keeps things clean and simple. > > Although it's more wordy, perhaps we should rename GetSVal() to > GetBinding() just to make the meaning of this method more clear. Agree.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
