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