martong added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
----------------
martong wrote:
> xazax.hun wrote:
> > I feel that we use different terms for the imported declarations. Sometimes
> > we call them `new`, sometimes `imported`, sometimes `foreign`. In case all
> > of these means the same thing, it would be nice to standardize on a single
> > way of naming. If there is a subtle difference between them, let's document
> > that in a comment. It would be nice if we did not need the comment after
> > the declaration but it would be obvious from the variable name.
> Yes, I agree that this should deserver some more explanation. Maybe right
> above this declaration?
>
> So, `new` means that a declaration is **created** newly by the ASTImporter.
> `imported` means it has been imported, but not necessarily `new`. Think about
> this case, we import `foo`'s definition.
> ```
> // to.cpp
> void bar() {} // from a.h
> // from.cpp
> void bar() {} // from a.h
> void foo() {
> bar();
> }
> ```
> Then `foo` will be `new` and `imported`, `bar` will be `imported` and not
> `new`.
> `foreign` basically means `imported` and `new`.
I've just added an explanatory comment for this field.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:432
+
+bool ExprEngine::ctuBifurcate(const CallEvent &Call, const Decl *D,
+ NodeBuilder &Bldr, ExplodedNode *Pred,
----------------
martong wrote:
> xazax.hun wrote:
> > What is the meaning if the return value? It looks like the function always
> > returns true.
> Nothing. It is being used by `inlineCall` but there are no callers of
> `inlineCall` that would use it because `inlineCall` does return true on all
> paths as well. Good point, this is again a rotten legacy. I am going update
> this soon.
I've changed the return value from bool to void, both in `ctuBifurcate` and in
`inlineCall`.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:516
+ // function in the main TU.
+ if (Engine.getCTUWorkList())
+ // Mark the decl as visited.
----------------
martong wrote:
> xazax.hun wrote:
> > So `getCTUWorkList` will return `nullptr` in the second phase? Reading this
> > code first I had the impression we will skip doing marking them visited in
> > the first phase. I think having a helper like `IsSecondCTUPhase` or
> > something would make this a bit less confusing.
> Fair enough. I am going to update.
I've added `isCTUInFirtstPhase()`.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:1130
// We are not bifurcating and we do have a Decl, so just inline.
- if (inlineCall(*Call, D, Bldr, Pred, State))
+ if (ctuBifurcate(*Call, D, Bldr, Pred, State))
return;
----------------
martong wrote:
> xazax.hun wrote:
> > I think this is getting really confusing here. The comment saying we are
> > not bifurcating and we call `ctuBifurcate`. While I do understand these are
> > two different kinds of bifurcation but I think we should rethink how to
> > name some of these functions.
> Yeah, good point, the comment is meaningless and confusing from this point,
> I'll just delete it.
I removed the comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123773/new/
https://reviews.llvm.org/D123773
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
