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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
