rsmith added a comment.

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.



================
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;
----------------
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).


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

Reply via email to