[PATCH] D114072: [clangd] Record IWYU pragma keep in the IncludeStructure

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> Sorry for forgetting about this one. Hopefully I can still help now by disagreeing with Kadir and creating an awkward stalemate instead.

Haha, always welcome!

> Partly because the ordering isn't some weird coincidence (see below)

Right, I suppose it makes sense when you look at the "construct being finalized" perspective.

> That's a more general approach for sure. But it does seem there's some complexity: what is the data structure and how do we merge it back into the graph. (e.g. are we going to record line numbers of each #include in the graph for this merging?)

Definitely, but we are not going to get rid of that extra complexity no matter which solution we go with. We still need to store some extra state about these comments, the only difference is when we process them & whether we store just the latest or the whole set.
Surely processing them in a different callback then `InclusionDirective` would add extra complexity, but I think it might also be simpler in general since we don't need to think about all the sequencing of these operations.
But after looking at the particular cases, I think I agree with your suggestion overall. Relying on the current sequencing to keep the state incremental rather than whole history sounds like the better trade off here. (details below).

> So let's think about the concrete cases:
>
> - export
> - begin_exports...end_exports
> - private maybe?
>
> The TL;DR is that each of these either stand alone, or we see the directive before the inclusion it describes in a way that's easy to keep track of incrementally. So I'm not really seeing much implementation difficulty to justify trying to move the directives and inclusion processing apart.

Yeah these are the cases I had in mind, but didn't really think about them thoroughly.
I guess in all of the export cases we just need to operate with the information we've seen before a particular inclusion directive, and the file including it. So there doesn't seem to be any need for postponing the processing. (Plus as you pointed out, keeping just the current state is definitely more simpler than keeping a set of those).

> Private I think we have to treat as a standalone directive that mutates the current file, and the reference to another file is treated as a string. It's not a file the PP has necessarily seen. So the result is a map<file, string> and it seems adequate for this to be written straight into IncludeStructure.

Yup, I guess this one looks pretty much the same for both approaches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072