[cfe-commits] [PATCH] getLValueElement

[Zhongxing Xu]

On Wed, Oct 22, 2008 at 11:19 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Oct 22, 2008, at 8:04 AM, Zhongxing Xu wrote:
>
>>
>> Is the idea for VisitCast to return the ElementRegion at index 0?
>>
>> Yes.
>>
>>  Where would this logic actually be performed?
>>
>> In GRExprEngine::VisitCast().
>>
>
> I should have been more specific.  I know that's when the logic happens,
> not just where the actual code lives.
>
>
>>
>>  VisitCast (in GRExprEngine) should have no notion of ElementRegion, as
>> that is something specific to a given Store implementation.  Do we need to
>> add another Store method that GRExprEngine::VisitCast calls back to?
>>
>> This is where I plan to add a new TransferFunction subclass corresponding
>> to RegionStore as GRSimpleVals to BasicStore. And in that TransferFunction,
>> EvalCast will do what is required to convert the VarRegion to ElementRegion
>> at index 0.
>>
>
> Sounds reasonable, but my overall feeling is that the whole transfer
> function mechanism needs to evolve, but that's not something we need to
> solve immediately.  Ideally, it would be nice if RegionStore could just be
> used by GRSimpleVals.  For example, the casting logic in GRSimpleVals seems
> fairly universal, and casts involving locations should probably just be
> forwarded on to the respective Store implementation.
>
> For example, having the method:
>
>  virtual SVal ArrayToPointer(SVal Array) { return Array; }
>
> in StoreManager would be sufficient, and just have VisitCast call that
> (RegionStore of course would override this).  That way we get a clean
> separation of concerns.
>

Yes, this is a better and simpler design.


>
> What I've noticed over time is the "assumption creep" that accrues in
> different pieces of libAnalysis, where one component will assume something
> about the implementation of another.  These assumptions usually lead to
> redundant code or making things less flexible then they should be.  Fixing
> these later usually ends up being a real pain.


Good point. I'll pay attention to this.


>
>
>  +SVal RegionStoreManager::getLValueElement(const GRState* St,
>> +                                          SVal Base, SVal Offset) {
>> +  if (Base.isUnknownOrUndef())
>> +    return Base;
>> +
>> +  Loc BaseL = cast<Loc>(Base);
>> +
>> +  switch (BaseL.getSubKind()) {
>> +  default:
>> +    assert("Other cases are not handled yet.");
>>
>> This assertion will always be true.  Did you mean assert(false && "...")?
>>
>> No, I just don't want the program to crash when things happen, but also
>> put reminder there.
>>
>
> Then it's not really an assertion.  Just use a comment.


OK. I'll make it a comment. But I do see others doing this in Clang.


>
>
>
>> Incidentally, do you need a switch statement, or do we plan on adding
>> support for bases other than MemRegion?
>>
>> I don't know whether bases other than MemRegion will happen in practice.
>> So I leave space for that. If we are sure no other cases, we can remove the
>> switch.
>>
>
> I'd prefer to keep the code cleaner, and change it to a switch statement
> later.


OK.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081023/10723e50/attachment.html>