xazax.hun added a comment.
Thanks! I have some questions inline.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:61
+ /// `D` must not be assigned a storage location.
+ void setStorageLocation(const VarDecl &D, StorageLocation &Loc) {
+ assert(VarDeclToLoc.find(&D) == VarDeclToLoc.end());
----------------
Are other language constructs coming in a follow-up PR? (E.g.: FieldDecl,
locations on the heap). I'd like to some TODO comments if that is the case. Or
if they are not supported by design, I'd love to some some comments about that.
Environment is referencing `ValueDecl` as opposed to `VarDecl`. What is the
reason for this asymmetry?
I think I overall find it confusing that some of the DeclToLoc mapping is here
some is in the Environment.
Does this has something to do with the fact that certain mappings should be the
same regardless of which basic blocks we are processing (i.e., invariant
globally so we do not need to duplicate it)? If that is the case please make it
clear in the comments.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:76
+ std::vector<std::unique_ptr<StorageLocation>> Locs;
+ std::vector<std::unique_ptr<Value>> Vals;
+ llvm::DenseMap<const VarDecl *, StorageLocation *> VarDeclToLoc;
----------------
Just curious: would it make sense to deduplicate these values?
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:33
+
+ virtual ~Value() {}
+
----------------
` = default;`
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:78
+
+ StorageLocation *getLocation() const { return Loc; }
+
----------------
I wonder if getLocation is ambiguous. (The pointer's own location vs the
pointee location.) How about something like `getPointeeLoc`?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:32
+template <typename K, typename V>
+bool denseMapsAreEqual(const llvm::DenseMap<K, V> &Map1,
+ const llvm::DenseMap<K, V> &Map2) {
----------------
Shouldn't we add `operator==` instead?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:46
+template <typename K, typename V>
+llvm::DenseMap<K, V> intersectDenseMaps(const llvm::DenseMap<K, V> &Map1,
+ const llvm::DenseMap<K, V> &Map2) {
----------------
I wonder if these algorithms should rather be somewhere in the support library.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:88
+ for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) {
+ FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
+ }
----------------
Could this end up creating an overly large state? There might be objects with
quite a lot fields but each function would only access a small subset of those.
Alternatively, we could attempt to create the representations for fields on
demand (this is the approach what the clang static analyzer is using).
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:117
+
+Value *Environment::initValueInStorageLocation(const StorageLocation &Loc,
+ QualType Type) {
----------------
I think this function implementation is better to be moved closer to the called
`initValueInStorageLocationUnlessSelfReferential` for readability.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:144
+ if (Type->isRealFloatingType()) {
+ auto &Value = DACtx->takeOwnership(std::make_unique<FloatValue>());
+ setValue(Loc, Value);
----------------
Are we planning to track float values in the near future? If not, would it make
sense to defer creating these objects until then?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:154
+ if (!Visited.contains(PointeeType)) {
+ Visited.insert(PointeeType);
+ initValueInStorageLocationUnlessSelfReferential(PointeeLoc, PointeeType,
----------------
Do we want to canonicalize types before inserting into this set?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:33
+static void transferDeclStmt(const DeclStmt &S, Environment &Env) {
+ if (const auto *D = dyn_cast<VarDecl>(S.getSingleDecl())) {
+ transferVarDecl(*D, Env);
----------------
Maybe add a TODO for `int a, b;` and similar codes?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:41
+
+ if (const auto *DS = dyn_cast<DeclStmt>(&S)) {
+ transferDeclStmt(*DS, Env);
----------------
Instead of using a long `if else` and dynamic casts, don't we want to use an
`StmtVisitor` instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116368/new/
https://reviews.llvm.org/D116368
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
