[PATCH] D58278: Prepare ground for re-lexing modular headers.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Lex/PPCallbacks.h:346
+
+  virtual void setPreprocessor(Preprocessor *preprocessor) {
+    preprocessor_ = preprocessor;
----------------
alexfh wrote:
> sammccall wrote:
> > This seems pretty tangly from a layering point of view, giving each object a reference to the other. I always find it hard to reason about ownership in such cases.
> > 
> > What about lifecycle methods `beginPreprocessing(Preprocessor*)` and `endPreprocessing(Preprocessor*)`? It will look similar in practice: subclasses that need it can still track the PP instance, but the circular references will only be there when needed, and will be explicit.
> We can do that as well. However, adding another couple of overrides to each PPCallbacks implementation will result in quite some boilerplate. How would you look at creating an intermediate class in the `PPCallbacks` -> <users' PPCallbacks handlers> hierarchy that will handle `beginPreprocessing`/`endPreprocessing` and expose the preprocessor pointer via a protected `getPreprocessor()` method?
I think that would probably obscure the intent, I really think this is subtle enough to be explicit, and would be only about as boilerplatey as today (the PP pointer is set by a method rather than in the constructor).

But I'm more concerned about the interfaces in Lex/ than the specific callsites in clang-tidy, so if such a helper class were private to clang-tidy, that seems ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58278