ymandel added a comment.
Herald added a subscriber: martong.
Overall, this seems to be an important API change, and I'm having trouble
seeing the big picture here. I think it would be great if you could expand (in
the CL description) on the motivation behind the change and the thinking behind
the particular design choices you've made here. For example, you mentioned
that this change supports explicit modeling of `nullptr`, but what is gained by
this explicit modeling? Also, regarding the design choice itself: is the intent
that native `nullptr` now represents an interpreted `nullptr`? If so, have you
considered instead introducing an explicit `NullptrLocation` or the like and if
so, what were the tradeoffs in this decision?
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:332
- StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const;
- const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
+ StorageLocation *skip(StorageLocation &Loc, SkipPast SP) const;
+ const StorageLocation *skip(const StorageLocation &Loc, SkipPast SP) const;
----------------
Please document the conditions under which these functions return nullptr.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:348
StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
SkipPast SP) const {
----------------
If I understand correctly, the `nullptr` result from this function is now
ambiguous: either there's no entry for that storagelocation, or there is and it
is null. But, perhaps I'm misunderstanding -- is this not a concern?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:266-267
- auto &Loc = Env.createStorageLocation(*S);
- Env.setStorageLocation(*S, Loc);
- Env.setValue(Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(
- SubExprVal->getPointeeLoc())));
+ // If PointeeLoc is null, then we are dereferencing a nullptr, skip
+ // creating a value for the dereference
+ if (auto *PointeeLoc = SubExprVal->getPointeeLoc()) {
----------------
sgatev wrote:
> Can you please add a test for this?
How does this work in practice? It seems to be an unsual case that we
statically know a pointer is null. So, what is the use of this change and (more
importantly) what is the impact on the framework, if any?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127746/new/
https://reviews.llvm.org/D127746
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
