martong marked 15 inline comments as done. martong added inline comments.
================ Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83 + + void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); } }; ---------------- whisperity wrote: > (The naming of this function feels a bit odd. `markAsNewDecl` or just > `markNewDecl`?) Good point, `markAsNewDecl` sounds better. I'll update this in the parent patch https://reviews.llvm.org/D123685 though (and rebase this). ================ 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: > 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. Thanks, this is so true, very good point. I've changed it. And also changed the name to suffix `-pct` instead of `-mul` to reflect this. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141 +enum class CTUPhase1InliningKind { None, Small, All }; + ---------------- whisperity wrote: > whisperity wrote: > > Is this configuration inherent to the static analyser, and not the > > //CrossTU// library? > (Documentation for the options are missing.) Yes, this is only for the analyzer. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:141 +enum class CTUPhase1InliningKind { None, Small, All }; + ---------------- martong wrote: > whisperity wrote: > > whisperity wrote: > > > Is this configuration inherent to the static analyser, and not the > > > //CrossTU// library? > > (Documentation for the options are missing.) > Yes, this is only for the analyzer. This is the direct representation of the analyzer option `ctu-phase1-inlining` and there is a bulky documentation for that in `AnalyzerOptions.def`. I'd rather not repeat that documentation here. ================ 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(); } ---------------- whisperity wrote: > 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? Yep, very good point, thanks again. Changed it with the inverted branch version. ================ 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 ---------------- whisperity wrote: > Why is there only 1 symbol in this file, when the file above contains two > function definitions? Good catch. I've added the other function. Besides, I had the wrong filename provided, it should have been `ctu-onego-existingdef-other.cpp.ast`, the `existingdef` was missing. Fixed that too, plus added and extra `FileCheck` to the `existingdef` test that detects if the ast file is not loaded. ================ 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 ---------------- whisperity wrote: > One-go? And what does that refer to? Is "Onego" analysis the one this patch > is introducing? Yes. I've updated the summary of the patch to make this clear. 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