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