[PATCH] D131280: [clang][dataflow] Parameterize analysis by explicit map of analyzable functions.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

In D131280#3706836 <https://reviews.llvm.org/D131280#3706836>, @xazax.hun wrote:

> The concept of fully qualified name is somewhat ambiguous for me. Do we expect the user to specify inline namespaces as well? I'd love to see some test cases and comments that clarify this.

Sure. This is probably worth some discussion. Fully qualified names, however we define them, will not be enough, since they don't cover overload sets. So, regardless, we need some internally-consistent (in clang) mechanism of uniquely identifying function declarations. Since we're also supporting methods and constructors, that implies IDs for classes as well.

I'd like some mechanism that matches how identifiers are used. So, for example, inline namespaces should *not* be necessary, since they are an implementation detail from this perspective.



================
Comment at: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp:96
+  for (auto It = Unit.top_level_begin(); It != Unit.top_level_end(); ++It) {
+    if (auto *C = dyn_cast<CXXRecordDecl>(*It)) {
+      for (auto *M : C->methods())
----------------
xazax.hun wrote:
> Do we exclude non-toplevel declarations on purpuse? Or would this work for methods of inline classes, methods of classes defined within a function? 
> Do we exclude non-toplevel declarations on purpuse? Or would this work for methods of inline classes, methods of classes defined within a function? 

I think that for the current use case -- models of library types and their methods/functions -- we don't have a good usecase for this. But, I can see this becoming an issue if we want to expand to inlining other declarations. So, I'm inclined to hold off on this for the time being, since that's a larger design discussion.

Yet, I also think this should be generalized to take any decl and extract the functions/methods. I limited to `ASTUnit` for convenience, since that was the immediate need. I'm happy to either:
1. Add a FIXME, and/or,
2. Split this function into two: one that takes two decl iterators (begin, end) and does this work here and another which is just a convenience function for ASTUnit to apply the above to the top-level decls.

WDYT?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:341
+  // Canonicalize the key, if possible.
+  if (auto *C = F->getCanonicalDecl())
+    F = C;
----------------
xazax.hun wrote:
> Out of curiosity, I'd expect getCanonicalDecl to always succeed. Do you know cases where it would not?
Hah. No, I don't but I also didn't find any mention in the headers guaranteeing otherwise, so I added this. I'm fine removing but I feel a little uncomfortable with the fact that it's not documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131280