dcoughlin added a comment.
Some comments on the C++ inline.
================
Comment at: include/clang/AST/ASTContext.h:82
class APValue;
+class ASTImporter;
class ASTMutationListener;
----------------
Is this forward declaration needed?
================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:63
private:
+ cross_tu::CrossTranslationUnitContext &CTU;
+
----------------
I don't think CrossTranslationUnitContext belongs in ExprEngine (it doesn't
really have much to do with transfer functions and graph construction.
Can you move it to AnalysisDeclContextManager instead?
Also, when you move it to AnalysisManager can you make it a pointer that is
NULL when naive cross-translation support is not enabled? This will make it
more clear that the cross-translation unit support will not always be available.
================
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:400
+ return CTUDir.getValue();
+}
----------------
Can you also add an analyzer option that is something like
'enable-naive-cross-translation-unit-analysis' and defaults to false?
I'd like to avoid using the presence of 'ctu-dir' as an indication that the
analyzer should use the naive CTU analysis. This way when if add a less naive
CTU analysis we'll be able to the CTUDir for analysis artifacts (such as
summaries) for the less naive CTU analysis as well.
================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:365
+
+ auto Engine = static_cast<ExprEngine *>(
+ getState()->getStateManager().getOwningEngine());
----------------
This downcast is an indication that the CTUContext is living in the wrong class.
================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:372
+
+ cross_tu::CrossTranslationUnitContext &CTUCtx =
+ Engine->getCrossTranslationUnitContext();
----------------
Can this logic be moved to AnalysisDeclContext->getBody()?
CallEvent::getRuntimeDefinition() is really all about modeling function
dispatch at run time. It seems odd to have the cross-translation-unit loading
(which is about compiler book-keeping rather than semantics) here.
================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:382
+ [&](const cross_tu::IndexError &IE) {
+ CTUCtx.emitCrossTUDiagnostics(IE);
+ });
----------------
I don't think it makes sense to diagnose index errors here.
Doing it when during analysis means that, for example, the parse error could be
emitted or not emitted depending on whether the analyzer thinks a particular
call site is reached.
It would be better to validate/parse the index before starting analysis rather
than during analysis itself.
================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:197
- AnalysisConsumer(const Preprocessor &pp, const std::string &outdir,
- AnalyzerOptionsRef opts, ArrayRef<std::string> plugins,
- CodeInjector *injector)
+ AnalysisConsumer(CompilerInstance &CI, const Preprocessor &pp,
+ const std::string &outdir, AnalyzerOptionsRef opts,
----------------
There is no need for AnalysisConsumer to take both a CompilerInstance and a
Preprocessor. You can just call `getPreprocessor()` to get the preprocessor
from the CompilerInstance
https://reviews.llvm.org/D30691
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
