NoQ added a comment.
@Szelethus, thank you a lot for covering this review!
@mate1214, yes, please please split this up, like @Szelethus described.
At the moment this patch is not only huge, it is very under-tested. Eg., the
Visitor promises support for C++ temporaries (which can potentially be a very
difficult problem), but tests don't contain even a single class. If the
functionality was added incrementally, it would have been easy to make sure
every improvement is covered by regression tests.
Am i understanding correctly that the visitor is capable of summing up stack
usage on the current path, i.e. ignoring objects that are put on the stack only
within parts of the function that were not in fact executed on the current
path, while also ignoring objects that are already removed from the stack? It
should indeed be possible to accomplish this by only looking at the AST (unless
VLAs or `alloca()` calls are used excessively), but in this case why is the
`checkEndFunction()` callback not specifying the current path?
I guess a good idea for this checker would be to add a `BugReporterVisitor` to
it in order to highlight where exactly does the stack usage go up. It is
allowed to be slow, so it might be reasonable to just outright re-run the
Visitor on every statement in the bug path.
I approve moving the visitor into `lib/Analysis`. That's indeed the library
into which we dump all static analysis that is not specific to Static Analyzer.
Once out of alpha, this checker will look great in `optin.performance`, where
we already have a checker for excessive structure padding.
================
Comment at:
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:28
+namespace clang {
+namespace stack {
+
----------------
Szelethus wrote:
> Use `ento` after `clang`. Btw, does anybody know what `ento` refers to? Never
> got to know it :D
> ```
> namespace clang {
> namespace ento {
> namespace stack {
It's Entomology.
Repository:
rC Clang
https://reviews.llvm.org/D52390
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
