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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Didn't really address this part

> But I don't think it matters in the long run especially when we want to handle different types of pragmas in the future
> So I'd suggest just building a side table information

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?)

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.

Export seems basically similar. We're going to see the pragma immediately before the directive.
The same stateful solution seems just as clear, I don't see any problem with it not being in the main file.
(There's the "same line" issue of having to look up line tables for everything, maybe we can dodge it by comparing SourceLocations in raw form instead, or use the statefulness to avoid checks in most cases)

Begin/end is a bit different. The stateful solution looks like: keep a `Set<FileID>` where exports are turned on, mutate it when you see a directive, and check it when you seen an inclusion.
The side-table solution is a bit more complicated: a map of fileIDs to a list of ranges or something. Manageable for sure.

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.



================
Comment at: clang-tools-extra/clangd/Headers.cpp:119
+  bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+    llvm::StringRef Text =
+        Lexer::getSourceText(CharSourceRange::getCharRange(Range),
----------------
This code will run for every comment in the preamble, so we should probably be careful about its performance.

I'd guess the cheapest thing is:
 - check that Range.begin() is not a macro
 - SM.getCharacterData(Range.begin()) yields a `char*` that's guaranteed null-terminated
 - try to consume the `IWYU pragma:` prefix

This doesn't deal with makeFileCharRange, touch End at all, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072