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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D106394#2922972 <https://reviews.llvm.org/D106394#2922972>, @rsmith wrote:

> I'm not in love with the design of this feature. Basing the accept / warn decision on whether the inclusion is in a system header seems fragile and doesn't seem to enforce the intended semantics of the warning (that you can only reach the implementation detail header through one of the named headers), and limits the utility of the feature to system headers. As an alternative, have you considered checking whether any of the headers named in the pragma are in the include stack, and warning if not? That would seem to make this warning applicable to both user headers and system headers in an unsurprising way. We'd presumably need two lists of headers: the external headers that we encourage people to include to get at the implementation detail header, and the internal list of headers that are also allowed to use it but that we shouldn't list in the diagnostic.

Wouldn't this suggestion mean that the following would be allowed?

  #include <utility>
  #include <__utility/move.h>

If not, then I'm probably misunderstanding your alternative design.



================
Comment at: clang/include/clang/Lex/HeaderSearch.h:116-122
+  /// List of aliases that this header is known as.
+  /// Most headers should only have at most one alias, but a handful
+  /// have two.
+  llvm::SetVector<llvm::SmallString<32>,
+                  llvm::SmallVector<llvm::SmallString<32>, 2>,
+                  llvm::SmallSet<llvm::SmallString<32>, 2>>
+      Aliases;
----------------
rsmith wrote:
> This is a very heavyweight thing to include in `HeaderFileInfo` -- it looks like this member will add a couple of hundred bytes to the struct. Please consider a more efficient way of storing these, such as storing a map from header to aliases in `HeaderSearch` and only including a single bit here to say if there are any aliases for a header (so we can avoid checking the map in the common case).
Good idea. I'll follow up with a patch some time this week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394