[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D106394#2893457 <https://reviews.llvm.org/D106394#2893457>, @ldionne wrote:

> In D106394#2892832 <https://reviews.llvm.org/D106394#2892832>, @sammccall wrote:
>
>> Eventually this seems like a reasonable thing to want for user code. What are your motivations for restricting it to system headers? (And do you think people in future will be tempted to lift them?)
>
> The problem with extending this to non-system headers is that you need a way to tell which headers are allowed to include the detail headers and which ones are not.

Ah, of course. It's a shame if this is a fatal reason because it seems like a small thing.

IWYU defines a whole bunch of other "pragmas", and one is "IWYU pragma: friend" for this purpose. This works but adds complexity to the source code (I didn't see it being as heavy as defining modules, but I haven't thought about it much) and also to consuming tools (we don't support it in clangd for this reason).

Annotating the include site instead like `#include "internals.h" // friend` seems almost tempting but magic-comments issues aside doesn't solve all the questions tools have (like what include to insert if none exists yet).

> Hmm... I like prior art. That clangd supports it suggests that there's a section of code I can look at for inspiration if we were to replace this pragma with the IWYU comment-pragma

Sure, there's nothing terribly interesting to see though. Clangd doesn't yet use it for warnings, but sometimes when we encounter an indexed symbol we consider inserting an `#include` for it (code completion, fixit for missing symbol). At indexing time the IWYU pragma is used to determine which insertable header we'll record for that symbol.

Indexing 1 <https://github.com/llvm/llvm-project/blob/f14495dc75d72d8900b3f4056a36a1ba5063e957/clang-tools-extra/clangd/index/CanonicalIncludes.cpp#L59-L82> - parse an IWYU comment. Note that it always produces a quoted string, which means a verbatim `#include` spelling, rather than a resolved file path, thus the FIXME.
Indexing 2 <https://github.com/llvm/llvm-project/blob/f14495dc75d72d8900b3f4056a36a1ba5063e957/clang-tools-extra/clangd/index/SymbolCollector.cpp#L228-L255> - determine the includable header for symbols from a FileID.
(The code completion and diagnostic fixit code isn't terribly interesting or readable...)

> (I wonder why they didn't just go with #pragma IWYU ...?).

Historically, it may have been the principled reason @Quuxplusone mentioned, or it may simply have been that as an *out-of-tree* clang-based tool, it's easier to hook comments in PPCallbacks than it is to hook an unknown pragma. (Of course these reasons are connected)

> If IWYU has already developed a de facto standard for this information, I would recommend libc++ just use it (perhaps with no changes needed in clang at all).

One hesitation I'd have about adopting this convention is that while the "IWYU pragma: friend" subset seems to coincide exactly with the design you want, the full set of IWYU pragmas are a rich/complicated set that I'm less confident in the design of.
Supporting only this subset will cause *some* confusion/pressure for more, but later choosing to extend the model will force a choice between adopting more IWYU pragmas (may be the wrong design) or doing something else with overlapping functionality (lots of confusion).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394