<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>I see what you mean about duplicated checks. Here's an idea: put all the checks in evalCastFromLoc under a special case for casting to bool. Then in evalBinOpLL:</div><div><br></div><div><font face="Courier">if (rInt->isZeroConstant()) {</font></div><div><font face="Courier"> if (op == BO_Sub)</font></div><div><font face="Courier"> return evalCastFromLoc(lhs, resultTy);</font></div><div><font face="Courier"> if (BinaryOperator::isComparisonOp(op))</font></div><div><font face="Courier"> return evalBinOpNN(state, op,</font></div><div><font face="Courier"> evalCastFromLoc(lhs, getContext().BoolTy),</font></div><div><font face="Courier"> makeTruthVal(false, getContext().BoolTy));</font></div><div><span style="font-family: Courier; ">}</span></div><div><br></div><div>That way you only have the ugly comparison once, and it's in SValBuilder rather than ConstraintManager.</div><div><br></div><div>Other comments:</div><div><br></div><div> QualType SymbolExtent::getType() const {<br> ASTContext &Ctx = R->getMemRegionManager()->getContext();<br>+<br>+ // If this symbol is wrapping a weak function region<br>+ // then the type should be a function pointer.<br>+ if (const FunctionTextRegion *FTR = dyn_cast<FunctionTextRegion>(R)) <br>+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(FTR->getDecl()))<br>+ if (FD->isWeak())<br>+ return Ctx.getPointerType(FD->getType());<br>+<br> return Ctx.getSizeType();<br> }<br><br></div><div>This is pretty ugly. I think it's better to just add a QualType field to SymbolExtent and have whoever constructs the object decide the type. It can even default to size_t for now (pending rename/refactoring).</div><div><br></div><div><br></div><div>+DefinedSVal SValBuilder::getWeakSymbolForRegion(const FunctionTextRegion *region) {<br>+ return nonloc::SymbolVal(SymMgr.getExtentSymbol(region));<br>+}<br>+<br><br></div><div>I think this can just return a symbol. Right now it's <i>not</i> really a nonloc, because the symbol type is a pointer. You can make it a SymbolData though to hide the implementation details, or even a fully-generalized SymbolRef.</div><div><br></div><div><br></div><div><div>+void testWeakFunc()</div><div>+{</div><div>+ clang_analyzer_eval(myFunc == NULL); // expected-warning{{FALSE}}</div><div>+ clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{UNKNOWN}}</div><div>+ if (myWeakFunc == NULL) {</div><div>+ clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{TRUE}}</div><div>+ }</div><div>+}</div></div><div><br></div><div>This is a good start, but you still need more tests: the false branch should have the answer FALSE, and the behavior should be the same if the condition is written 'myWeakFunc' or '!myWeakFunc'.</div><div><br></div><div>Getting there...!</div><div>Jordan</div><div><br></div><br><div><div>On Jan 23, 2013, at 11:23 , Richard <<a href="mailto:tarka.t.otter@googlemail.com">tarka.t.otter@googlemail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>So you mean something like the attached diff? I am not so sure, it makes some things simpler, but I don't like the duplicated messy checks for weak function regions in SimpleSValBuilder and SimpleConstraintManager. What do you reckon? I actually remembered to include a working test this time too.</div><div><br></div><div></div></div><span><latest.diff></span><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div></div><br><div><div>On 21 Jan 2013, at 18:48, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jan 20, 2013, at 9:00 , Richard <<a href="mailto:tarka.t.otter@googlemail.com">tarka.t.otter@googlemail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite"><div>+</div><div>+ void setWeakSymbol(const SymbolExtent *S) {</div><div>+ // Only weak functions can have symbols.</div><div>+ assert(cast<ValueDecl>(FD)->isWeak());</div><div>+ WeakSym = S;</div><div>+ }</div></blockquote><div><br></div><div>MemRegions need to be immutable, so the setWeakSymbol() is not going to work. Can we just have the symbol associated with the FunctionTextRegion when it gets created?</div></div></blockquote><div><br></div><div>I would like to do this, but I don't see how it is possible easily. The SymbolExtent requires the FunctionTextRegion in its constructor, and I don't see a nice way to get hold of the SymbolManager in the FunctionTextRegions constructor to create it. Any suggestions here?</div></div></blockquote><div><br></div><div>My new thought on this is that this should just be created lazily by SValBuilder and that it doesn't even need to live on the region. SValBuilder::getWeakSymbolForRegion, or something like that. </div><div><br></div><div>I'll talk to Anna and Ted about what to rename SymbolExtent and SymbolMetadata to properly include this new usage; if you have any opinions, I'd love to hear them!</div><div><br></div><div>SymbolExtent:</div><div>- based on the region itself</div><div>- does not get invalidated</div><div>- live as long as the region is live (in theory)</div><div><br></div><div>SymbolMetadata:</div><div>- based on the region contents</div><div>- is invalidated along with region contents</div><div>- live as long as the region is live <i>and</i> some checker is interested</div><div><br></div><div>Jordan</div></div></div></blockquote></div><br></div></blockquote></div><br></body></html>