NagyDonat wrote:
> You probably shouldn't write a lot of code that you're about to delete
> anyway. So in my opinion it doesn't make sense to make a unit test or a debug
> checker just for this patch.
>
> Maybe just assert that the states are the same before you pass it to the
> checker in evalCall? It's very slightly unobvious there with the whole loop
> thing and that's where the problem was anyway. Or just leave it as-is.
I think I will leave it as-is because the initialization of `UpdatedCall` and
the `CheckerContext` are really close and they are both relying on the same
`Pred` state:
```c++
ProgramStateRef State = Pred->getState();
CallEventRef<> UpdatedCall = Call.cloneWithState(State);
// Check if any of the EvalCall callbacks can evaluate the call.
for (const auto &EvalCallChecker : EvalCallCheckers) {
// TODO: Support the situation when the call doesn't correspond
// to any Expr.
ProgramPoint L = ProgramPoint::getProgramPoint(
UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind,
Pred->getLocationContext(), EvalCallChecker.Checker);
bool evaluated = false;
{ // CheckerContext generates transitions (populates checkDest) on
// destruction, so introduce the scope to make sure it gets properly
// populated.
CheckerContext C(B, Eng, Pred, L);
evaluated = EvalCallChecker(*UpdatedCall, C);
}
```
> If you won't be able to do the follow-up commit then you can add a clever
> follow-up test instead.
I'm pretty sure that I will do the follow-up commit -- it is probably simpler
than designing a testcase.
------
> Actually maybe just keep the strict bare minimum of information in the call
> event? Like a `SmallVector<SVal, ...>` of args, a potential return value as
> `Optional<SVal>`, the runtime definition that's been already computed
> elsewhere as a `Decl *`, and more subclass-specific stuff in the subclasses.
>
> If you don't want the data to go out of sync, just normalize your database.
I would really like to have `CallEvent` implemented this way (I strongly agree
that this would be more elegant than the status quo), but I fear that
_reaching_ this ideal state would be a difficult undertaking -- especially on
the weird Objective-C areas that I really don't understand.
I strongly support this "keep the bare minimum in the call event" idea as a
long-term development plan, but I cannot promise to implement it in the
foreseeable future, so I would still like to merge this PR as a temporary
solution. (If call events do end up being minimized, then it will be possible
to discard the logic tweaked in this PR along with all the other code that
updates the states attached to `CallEvent`s.)
Also note that this development plan is compatible with my other plans (making
`CallEvent::getState()` protected and ensuring that
`CallEvent::invalidateRegions()` always takes the state explicitly) because
those are necessary first steps towards implementing a setup where `CallEvent`
doesn't have a full state (not even as a private member).
https://github.com/llvm/llvm-project/pull/160707
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
