ymandel added a comment. In D131280#3706988 <https://reviews.llvm.org/D131280#3706988>, @xazax.hun wrote:
> 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 Thanks. That looks good, but I'm concerned that it only counts the arguments and doesn't look at their types. I'd imagine this will be a limitation down the line when we want to deal with overload sets w/ the same number of arguments, but different types. Aside: why the `const char *` interface? Do you think owners would be open to a `llvm::StringRef` overload for the constructor? ================ 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: > 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. Thanks, done. ================ Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:549 - // Note that it is important for the storage location of `S` to be set - // before `pushCall`, because the latter uses it to set the storage - // location for `return`. - auto &ReturnLoc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, ReturnLoc); - auto CalleeEnv = Env.pushCall(S); + const FunctionDecl *FuncDecl = CFCtx->getDecl()->getAsFunction(); + assert(FuncDecl != nullptr && "ControlFlowContexts in the environment " ---------------- sgatev wrote: > How is that different from `F`? Why not let `Environment::pushCall` get this > from the `CallExpr` argument? Added comment to explain. 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
