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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
