[PATCH] D136668: [clang][dataflow] Add initial sign analysis

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Nice!!  Just nits.

At a high level, I'm a little concerned about this as a demo, since I wouldn't recommend this implementation in practice  (for various reasons, e.g. I would encode with 2 booleans since there are only 3 values). But, I think this approach has the advantage of clarity over optimized approaches. It might be worth pointing this out in comments at places where you made a decision for purposes of clarity/readability, if any come to mind.



================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:62
+struct SignProperties {
+  BoolValue *Neg, *Zero, *Pos;
+};
----------------
please indicate ownership in comment


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:92
+
+std::tuple<Value *, SignProperties, SignProperties>
+getValueAndSignProperties(const UnaryOperator *UO,
----------------
Please comment on the meaning of the tuple. Even better, define a (named) struct.
In general, this function could use a comment explaining what it does. 


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:113
+  // Properties for the operand (sub expression).
+  SignProperties OpdPs = getSignProperties(*OperandVal, State.Env);
+  if (!OpdPs.Neg)
----------------
Please expand this out a bit. e.g. `OperandProps`. `Opd` is not a familiar/common abbreviation, so I think its use harms readability.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:114
+  SignProperties OpdPs = getSignProperties(*OperandVal, State.Env);
+  if (!OpdPs.Neg)
+    return {nullptr, {}, {}};
----------------
Given the boolean nature of these properties, it's really easy to misread these pointer operations as boolean operations, and be confused (at least, easy for me to be confused, so I'm being selfish :)).


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:134
+
+  // TODO Use this as well:
+  // auto *NegatedComp = &State.Env.makeNot(*Comp);
----------------
FIXME


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:143
+
+  SignProperties LHSPs = getSignProperties(*LHS, State.Env);
+  SignProperties RHSPs = getSignProperties(*RHS, State.Env);
----------------
perhaps something more readable, like `LHSProps` or `LeftProps`?


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:145
+  SignProperties RHSPs = getSignProperties(*RHS, State.Env);
+  if (!LHSPs.Neg || !RHSPs.Neg)
+    return;
----------------
please expand pointer operation (as above)


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:183
+    break;
+  case BO_NE: // Noop.
+    break;
----------------
why not? its a test, so not necessary, just curious to know why you're leaving this out.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:193
+                        LatticeTransferState &State) {
+  auto [UOVal, UOPs, OpdPs] = getValueAndSignProperties(UO, M, State);
+  if (!UOVal)
----------------
same comments as above about variable naming.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:280
+
+      // TODO handle binop +,-,*,/
+
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:396
+
+// FIXME add this to testing support.
+template <typename NodeType, typename MatcherType>
----------------
It doesn't appear to be used.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:398
+template <typename NodeType, typename MatcherType>
+const NodeType *findFirst(ASTContext &ASTCtx, const MatcherType &M) {
+  auto TargetNodes = match(M.bind("v"), ASTCtx);
----------------
Given the assertion on line 400, I think this name shouldn't be `First`. Maybe `Only`, or `Unique`?


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:425
+
+// If Test if the given property of the given node is implied by the flow
+// condition. If 'Implies' is false then check if it is not implied.
----------------
typo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136668