Charusso added a comment.
Thanks for the review! I am not sure why but after your review I always see the
most appropriate design immediately.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1253-1255
+ MemRegionManager(ASTContext &c, llvm::BumpPtrAllocator &a, SValBuilder &SVB,
+ SymbolManager &SymMgr)
+ : Ctx(c), A(a), SVB(SVB), SymMgr(SymMgr) {}
----------------
NoQ wrote:
> This looks like a layering violation to me. It's not super important, but i'd
> rather not have `MemRegion` depend on `SValBuilder`.
>
> Can we have `getStaticSize()` be a method on `SValBuilder` instead? Or simply
> a standalone static function in `DynamicSize.cpp`? 'Cause ideally it
> shouldn't be called directly.
Hm, in my game-dev world every manager knows about every manager, so I felt
that it needs to work. I like the idea behind the directness and hiding the
implementation, but I believe the `MemRegionManager` should manage its stuff.
Also we are lucky, because the `SValBuilder` is available everywhere with a
tiny stress on the API.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp:95-99
+ DefinedOrUnknownSVal DynSize = getDynamicSize(state, R);
+
+ DefinedOrUnknownSVal DynSizeMatchesSizeArg =
+ svalBuilder.evalEQ(state, DynSize,
Size.castAs<DefinedOrUnknownSVal>());
+ state = state->assume(DynSizeMatchesSizeArg, true);
----------------
NoQ wrote:
> As the next obvious step for the next patch, i suggest replacing `evalEQ()`
> with some sort of `setDynamicSize()` here.
Okay, good idea, thanks! I want to eliminate `getSizeInElements` as well.
================
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:
> > 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!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
