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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D106394#2891647 <https://reviews.llvm.org/D106394#2891647>, @zoecarver wrote:

> Can't wait to start using this! (Note: this is not a full review, just some thoughts.)
>
> Do you have a test case where
>
>   // private: header one
>   *exists*
>   
>   // private: header two
>   #include<__header_one.h>
>   
>   // public: header three
>   #include<__header_two.h>
>
> I think right now you might see a warning that says "please include either `__header_two.h` or `header_three.h`" which is not right.

Right now, I've got `please include either '<public_header_1.h>' or '"publich_header_2.h"'` and `please include one of {'<public_header_1.h>', '"publich_header_2.h"', '<private_header_1.h>'}`. I suppose I should add another public header.

> This begs the question, would it not be better to have a second argument that specifies what header should be used as a replacement? This would also allow the person writing the library to decide whether it's better to use angle brackets or quotes.

This is the purpose of the first argument, so I'm not sure I follow.



================
Comment at: clang/include/clang/Lex/HeaderSearch.h:465
+        getFileInfo(File).Aliases;
+    auto InsertionPoint = llvm::lower_bound(Aliases, Alias);
+    if (InsertionPoint != Aliases.end() && *InsertionPoint == Alias)
----------------
zoecarver wrote:
> Is the logic here is just "if it's not already here add it?" If so, I think maybe you should just make `Aliases` a set (either a `StringSet` or `SmallSet`). Do you care about order? 
Yes. I was going to use `StringSet`, but need `operator[]` for diagnostics. `SetVector` was my next choice, but it was giving me some serious complaints about a `DeepMap` or something. I guess I could copy the `StringSet`'s contents into a `SmallVector` at the diagnostic site, but I really don't expect headers to have more than two aliases?


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:429
+        };
+        std::string Aliases =
+            std::accumulate(Info.Aliases.begin() + 1, Info.Aliases.end(),
----------------
zoecarver wrote:
> Can we use `llvm::interleaveComma` or `interleave`?
Thanks, I was looking for `join`!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106394