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