[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

Wed Dec 23 20:16:22 PST 2020

NoQ added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:20-21
 #include "clang/Basic/Version.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Lex/Preprocessor.h"
----------------
steakhal wrote:
> NoQ wrote:
> > Will these two includes be ultimately removed? Like, even in the CTU case?
> I think `PlistDiagnostics` can depend on `CTU`.
> `ASTUnit` is a different thing though. I think I just accidentally left that include.
> 
> I will remove the `#include "clang/Frontend/ASTUnit.h"` line.
> 
> ---
> What do you mean by the 'CTU case'?
> I think PlistDiagnostics can depend on CTU.

So, like, my problem is that i want to move all `PathDiagnosticConsumer`s (with implementations) into `libAnalysis` so that other tools could use it but `libAnalysis` can't link-time-depend on `libCrossTU` because that'd be a circular dependency. Basically `libAnalysis` is a tiny library that contains analyses whereas `libCrossTU` is a high-level library that manages entire clang invocations. Unless we rethink our library hierarchies entirely, i believe we're stuck with this constraint.

Removing the dependency on `libFrontend` is insufficient because `libCrossTU` depends on `libFrontend` anyway. 

If you make `MacroExpansionContext` abstract and put the concrete implementation in `libCrossTU`, thus replacing my (currently reverted) attempt in D92432 to break the dependency, that'd fix the problem entirely. Otherwise i'll probably wait for your work to land and do this myself anyway.

> What do you mean by the 'CTU case'?

Nothing really (: It was an attempt to emotionally highlight the importance of not having a link-time dependency on `libCrossTU` even while handling the CTU execution path, according to whatever definition of that you were alluding to in the review summary.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93224