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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:56
+
+  /// Assigns `Loc` to `D`.
+  ///
----------------
ymandel wrote:
> 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`?
I opted for "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.
+  ///
----------------
ymandel wrote:
> 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?
I replaced the pointers with references where possible.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:40-42
+private:
+  Kind LocKind;
+  QualType Type;
----------------
ymandel wrote:
> 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.
Ack. I prefer this order because it puts the public interface higher so the important bits can be found more easily. If this goes against the LLVM coding style I'd be happy to change it, but I prefer to do this separately because I already used this style in some of the other dataflow classes.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:52
+
+  static bool classof(const StorageLocation *Loc) {
+    return Loc->getKind() == Kind::Scalar;
----------------
ymandel wrote:
> ref?
I believe this needs to accept a pointer to be compatible with operations defined in `llvm/Support/Casting.h`.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:37-38
+
+private:
+  Kind ValKind;
+};
----------------
ymandel wrote:
> Move to beginning of class decl?
Ack. I'll apply this separately if necessary.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:41
+
+/// Class that models an integer.
+class IntegerValue : public Value {
----------------
ymandel wrote:
> nit: Here and below. I think that "Models..." would be as good or better than "Class that models...".
Yeap, that's better. Updated all comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116368