whisperity added inline comments.
================ Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83 + + void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); } }; ---------------- (The naming of this function feels a bit odd. `markAsNewDecl` or just `markNewDecl`?) ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413 + unsigned, CTUMaxNodesMultiplier, "ctu-max-nodes-mul", + "We count the nodes for a normal single tu analysis. We multiply that " + "number with this config value then we divide the result by 100 to get " ---------------- ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:413-419 + "We count the nodes for a normal single tu analysis. We multiply that " + "number with this config value then we divide the result by 100 to get " + "the maximum number of nodes that the analyzer can generate while " + "exploring a top level function in CTU mode (for each exploded graph)." + "For example, 100 means that CTU will explore maximum as much nodes that " + "we explored during the single tu mode, 200 means it will explore twice as " + "much, 50 means it will explore maximum 50% more.", 100) ---------------- whisperity wrote: > Couldn't this description here be simplified to say something along the lines of //"the percentage of single-TU analysed nodes that the CTU analysis is allowed to visit"//? Is the calculation method needed from the user's perspective? The examples talk about percentage too. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141 +enum class CTUPhase1InliningKind { None, Small, All }; + ---------------- Is this configuration inherent to the static analyser, and not the //CrossTU// library? ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141 +enum class CTUPhase1InliningKind { None, Small, All }; + ---------------- whisperity wrote: > Is this configuration inherent to the static analyser, and not the > //CrossTU// library? (Documentation for the options are missing.) ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:156 llvm::PointerUnion<const Expr *, const Decl *> Origin; + mutable Optional<bool> Foreign; // From CTU. ---------------- ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:814-816 + /// Returns true if the CTU analysis is running its first phase. + /// Returns true in single TU (non-CTU) mode! + bool isCTUInFirtstPhase() { return Engine.getCTUWorkList(); } ---------------- How and why is this needed? Could you call it `isSingleTUOr1stPhaseCTU` instead? Rather, if this is used as a distinction input in conditionals, could you invert the branches and have a function `isSecondPhaseCTU`, and do the inverted logic where this function is consumed? ================ Comment at: clang/test/Analysis/Inputs/ctu-onego-existingdef-other.cpp.externalDefMap.ast-dump.txt:1 +11:c:@F@other# ctu-onego-other.cpp.ast ---------------- Why is there only 1 symbol in this file, when the file above contains two function definitions? ================ Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:22 // +// FIXME On-demand ctu should be tested in the same file that we have for the +// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu) ---------------- ================ Comment at: clang/test/Analysis/ctu-on-demand-parsing.cpp:33 + +// FIXME On-demand ctu should be tested in the same file that we have for the +// PCH version, but with a different verify prefix (e.g. -verfiy=on-demand-ctu) ---------------- ================ Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:24 +// RUN: -verify=ctu %s 2>&1 | FileCheck %s +// +// CallGraph: c->b ---------------- Are the lines below related to the execution of the command above? If not, could you please break the comment? ================ Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:28 +// Note that `other` calls into `b` but that is not visible in the CallGraph +// b/c that happens in another TU. + ---------------- ================ Comment at: clang/test/Analysis/ctu-onego-toplevel.cpp:30 + +// During the onego CTU analysis, we start with c() as top level function. +// Then we visit b() as non-toplevel during the processing of the FWList, thus ---------------- One-go? And what does that refer to? Is "Onego" analysis the one this patch is introducing? 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