xazax.hun added a subscriber: gribozavr2. xazax.hun added a comment. In D131280#3706915 <https://reviews.llvm.org/D131280#3706915>, @ymandel wrote:
> 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. I see. This is not a unique problem. I think there were multiple discussions about API Notes and those need to solve the same problem. @gribozavr2 probably has more context on the current status of API Notes. An alternative to fully qualified names is Clang's USR that is often used for cross-referencing functions across translation units. Less user friendly, but will support overloads. > 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. A similar matching is already implemented for the Clang Static Analyzer. See https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CallDescription.html and https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CallDescriptionMap.html One example use is in the CStringChecker: https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L136 ================ 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()) ---------------- ymandel wrote: > 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? I am fine with the current behavior, but I think the docstring "Builds a map of all declared functions in the given AST" is misleading in this case. Specifying in the docstring that this function will only map the top-level functions and methods of top-level classes sounds good to me. ================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:341 + // Canonicalize the key, if possible. + if (auto *C = F->getCanonicalDecl()) + F = C; ---------------- ymandel wrote: > 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. I see other places where we don't check for nullness, e.g.: https://github.com/llvm/llvm-project/blob/bb297024fad2f6c3ccaaa6a5c3a270f73f15f3ac/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp#L83 I think the CanonicalDecl is defined as the first declaration for redeclarable entities. I think getting the first element of a non-empty list should always succeed. But I definitely agree, this fact should be documented, or even better, this should probably return a reference. 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
