[PATCH] D104616: [analyzer] Model comparision methods of std::unique_ptr

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun requested changes to this revision.
xazax.hun added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:350
+    QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+    std::tie(Val, NewState) = retrieveOrConjureInnerPtrVal(Reg, E, Type, C);
+    return {Val, NewState};
----------------
Nit: couldn't you just ` return retrieveOrConjureInnerPtrVal(Reg, E, Type, C);` instead of all the ceremony with `std::tie`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:365
+  std::tie(FirstPtrVal, State) = makeSValFor(FirstExpr, First);
+  std::tie(SecondPtrVal, State) = makeSValFor(SecondExpr, Second);
+  BinaryOperatorKind BOK =
----------------
I think we might end up losing some information here. Imagine both `makeSValFor` conjuring a new symbol. Only one of the symbols will be stored in the state since capturing happend once, when you created the lambda. Maybe it is better to ask for a state in `makeSValFor` as a parameter instead of doing a lambda capture. 


Moreover, `retrieveOrConjureInnerPtrVal` will not even look at the state captured by `makeSValFor`. It will ask the `CheckerContext` to get the state, but that state is the state from the beginning of the function that does not have any of the modifications you made to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616