[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
               // symbols to use, only content metadata.
-              return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+              return FTR->getMemRegionManager().getStaticSize(FTR);
 
----------------
NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > That is the breaking test's code, which is super wonky. I cannot understand what is the rational behind this concept.
> > > Your new code would return a concrete integer here:
> > > ```lang=c++
> > >   case MemRegion::FunctionCodeRegionKind: {
> > >     QualType Ty = cast<TypedRegion>(SR)->getDesugaredLocationType(Ctx);
> > >     return getTypeSize(Ty, Ctx, SVB);
> > >   }
> > > ```
> > > Previously it was a symbol.
> > > 
> > > That said, the original code looks like a super gross hack: they used an extent symbol not because they actually needed an extent, but because they didn't have a better symbol to use :/ I guess you should just keep the extent symbol for now :/
> > I see, that was really missing, whoops. Thanks!
> Let's keep the hack here, not move it to the region manager. I.e., please create `SymbolExtent` here directly and then separately keep creating it in `getStaticSize()`. I.e., don't reuse the code when it isn't supposed to behave identically, even if right now it accidentally does behave identically.
Ah, yes, it makes sense. In my mind they are connecting even in fixing them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69540/new/

https://reviews.llvm.org/D69540