NoQ added a comment.

Had a fresh look on the C++ part, it is super clean now, i'm very impressed :)



================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:373-374
 
-  return RuntimeDefinition();
+  auto Engine = static_cast<ExprEngine *>(
+      getState()->getStateManager().getOwningEngine());
+
----------------
*Humbly suggests to refactor whatever we need here into `SubEngine`'s virtual 
method(s).

`getAnalysisManager()` is already there, so i guess we only need to expose 
`getCrossTranslationUnitContext()`.


================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:385
+          FD, Engine->getAnalysisManager().options.getCTUDir(),
+          "externalFnMap.txt");
+
----------------
I think `CallEvent` is the last place where i'd prefer to hardcode this 
filename. Maybe hardcode it in `CrossTranslationUnitContext` or get from 
`AnalyzerOptions`? (the former, i guess, because i doubt anybody would ever 
want to change it).


================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:418-423
     SourceLocation XDL = XD->getLocation();
     SourceLocation YDL = YD->getLocation();
     if (XDL != YDL) {
       const SourceManager &SM = XL.getManager();
-      return SM.isBeforeInTranslationUnit(XDL, YDL);
+      return compareCrossTUSourceLocs(FullSourceLoc(XDL, SM),
+                                      FullSourceLoc(YDL, SM));
----------------
It seems to me that `XDL` and `YDL` are exactly the same as `XL` and `YL` we've 
seen at the beginning of the function.

...we still have only one `SourceManager`, right?


https://reviews.llvm.org/D30691



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to