[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144
+    // includes the full path.
+    if (SM.getFilename(IL).contains("UnifiedSource")) {
+      StringRef Name = SM.getFilename(SL);
----------------
Is this `if` really necessary? This logic has too much overfitting, and it seems that if someone decides to include `.cc` files, we should analyze them in any case, right? We also would prefer to not stop working if webkit decides on using a different naming for those.


================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:975
+                                           ->getAnalysisManager()
+                                           .isInCodeFile(D->getLocation()))
       return true;
----------------
You would shave off a bit of redundancy and verbosity by saving `AnalysisManager` in a local variable.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:837
             return false;
 
       // Conditionally control the inlining of the destructor of C++ shared_ptr.
----------------
baloghadamsoftware wrote:
> Maybe we should include a test for container methods as well.
+1


================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:153
+  assert(AnalysisManager::isInCodeFile(CallLoc, SMgr) &&
          "The call piece should be in the main file.");
 
----------------
Assert error message is not quite technically correct now


================
Comment at: test/Analysis/unified-sources/source1.cpp:8
+  if (x) {}
+  return 1 / x; // expected-warning{{}}
+}
----------------
Wow, expected-* directives work across multiple files?? This is really cool!


https://reviews.llvm.org/D45839