[PATCH] D116368: [clang][dataflow] Add transfer function for VarDecl statements

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Nice!  A few small comments on the headers...



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:56
+
+  /// Assigns `Loc` to `D`.
+  ///
----------------
The term "assignment" is overloaded. :) Maybe instead "Associates `Loc` with `D`"? Or, expand on the meaning of the assignment? e.g. "Assigns `Loc` as the storage location of `D`?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:41
+  /// Creates a storage location appropriate for `Type`. Does not assign a value
+  /// to the returned storage location in the environment. Never returns null.
+  ///
----------------
here and below. If it never returns null, why not return a ref?
For that matter, all of the pointer parameters with comments "must not be null" -- can we just make those refs?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:9
+//
+// This file defines classes that represent elements of the local variable store
+// and of the heap during dataflow analysis.
----------------
Indent two spaces? (vs 1) same below.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:40-42
+private:
+  Kind LocKind;
+  QualType Type;
----------------
I don't think its required, but LLVM style typically puts private field decls at the beginning of the class, without the explicit "private:" decl.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:52
+
+  static bool classof(const StorageLocation *Loc) {
+    return Loc->getKind() == Kind::Scalar;
----------------
ref?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:83
+    auto It = Children.find(D);
+    // FIXME: A missing child member means that the sorage location was not
+    // created correctly. Change the following to assert(it != children_.end())
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:37-38
+
+private:
+  Kind ValKind;
+};
----------------
Move to beginning of class decl?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:41
+
+/// Class that models an integer.
+class IntegerValue : public Value {
----------------
nit: Here and below. I think that "Models..." would be as good or better than "Class that models...".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116368