<br><br><div class="gmail_quote">On Wed, Oct 22, 2008 at 11:19 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d"><br>
On Oct 22, 2008, at 8:04 AM, Zhongxing Xu wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Is the idea for VisitCast to return the ElementRegion at index 0?<br>
<br>
Yes.<br>
<br>
 Where would this logic actually be performed?<br>
<br>
In GRExprEngine::VisitCast().<br>
</blockquote>
<br></div>
I should have been more specific.  I know that's when the logic happens, not just where the actual code lives.<div class="Ih2E3d"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
 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?<br>
<br>
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.<br>

</blockquote>
<br></div>
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.<br>

<br>
For example, having the method:<br>
<br>
  virtual SVal ArrayToPointer(SVal Array) { return Array; }<br>
<br>
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.<br>
</blockquote><div><br>Yes, this is a better and simpler design.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
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.</blockquote>
<div><br>Good point. I'll pay attention to this.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+SVal RegionStoreManager::getLValueElement(const GRState* St,<br>
+                                          SVal Base, SVal Offset) {<br>
+  if (Base.isUnknownOrUndef())<br>
+    return Base;<br>
+<br>
+  Loc BaseL = cast<Loc>(Base);<br>
+<br>
+  switch (BaseL.getSubKind()) {<br>
+  default:<br>
+    assert("Other cases are not handled yet.");<br>
<br>
This assertion will always be true.  Did you mean assert(false && "...")?<br>
<br>
No, I just don't want the program to crash when things happen, but also put reminder there.<br>
</blockquote>
<br></div>
Then it's not really an assertion.  Just use a comment.</blockquote><div><br>OK. I'll make it a comment. But I do see others doing this in Clang.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Incidentally, do you need a switch statement, or do we plan on adding support for bases other than MemRegion?<br>
<br>
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.<br>
</blockquote>
<br></div>
I'd prefer to keep the code cleaner, and change it to a switch statement later.</blockquote><div><br>OK. <br></div></div><br>