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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
