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

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 9 10:02:26 PST 2020


NoQ 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:
----------------
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).


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

https://reviews.llvm.org/D92432



More information about the cfe-commits mailing list