[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:124
 /// Note that this class also implements caching.
-class CrossTranslationUnitContext {
+class CrossTranslationUnitContext : public CrossTUAnalysisHelper {
 public:
----------------
NoQ wrote:
> steakhal wrote:
> > martong wrote:
> > > Why don't we have a dependency in libCrossTU to libAnalysis? In the CMakeLists.txt?
> > > Here we implement the CrossTUAnalysisHelper's abstract virtual function thus we include the `CrossTUAnalysisHelper.h`. But 
> > > CMake should know about the dependency even if this is only a header only dependency, shouldn't it? (We were talking about this with @steakhal.)
> > IMO if a library (A) does not depend on another library (B), that means we can safely delete all files of the **B** and still be able to compile and run the **A**.
> > In this case, it won't, as the given header lives under **B**. So to make sure this principle works, we should state the dependency on **B** in **A**.
> > 
> > ---
> > In our case, it means that **libCrossTU** depends on **libAnalysis**, as the `CrossTUAnalysisHelper` lives in `libAnalysis` which is used by `libCrossTU` to implement the `CrossTranslationUnitContext`.
> There's already an indirect dependency (you have to look at all those CMakeLists.txt in order to notice it): libCrossTU -> libFrontend -> libSema -> libAnalysis (and a few other paths through the dependency graph). There's no harm in writing it down explicitly but there's also no explicit need and i'd rather have it not specified directly (if there's too little dependencies it'll warn us) than to specify too much by accident (there's no warning for unused dependencies and if a dependency eventually becomes unused it may accidentally prevent people from adding the dependencies they actually need).
Thank you for the explanation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92432/new/

https://reviews.llvm.org/D92432