[PATCH] D38639: [clangd] #include statements support for Open definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D38639#920427, @malaperle wrote:

> I'm not sure it's better to use the InclusionDirective callback for this. We need to get the includes in two places: in the preamble and non-preamble. In the preamble we use the callback, have to store some temporary stuff because we don't have a SourceManager in InclusionDirective, then in finish we use the SourceManager to convert everything. In the non-preamble, we cannot use the callback so we use the SourceManager to go through the includes. Therefore, we have to maintain two different ways of getting the inclusion map. Without using the InclusionDirective callback, we use the SourceManager in both cases and can use the same code to iterate through inclusions, just on two different SourceManagers at different moments. We also don't need to make another patch to modify the PreambleCallbacks. I also double this is much slower but I think this simplifies the code nicely.
>  What do you think?


I think we should never iterate through `SourceManager`, as it's much easier to get wrong than using the callbacks. I agree that all that fiddling with callbacks is unfortunate, but it's well worth the fact that it'd be much easier to tell that the implementation is correct by simply looking at the implementation and not knowing how `SourceManager` works. `PPCallbacks` is a much more direct API that was designed to handle our purposes.

Note that we don't need to use `SourceManager` to find non-preamble includes, we should implement proper `PPCallbacks` and use them when building the AST.


https://reviews.llvm.org/D38639