[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Devin Coughlin via Phabricator via cfe-commits]

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