VitaNuo added inline comments.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:20
static llvm::StringRef *HeaderNames;
-static std::pair<llvm::StringRef, llvm::StringRef> *SymbolNames;
-static unsigned *SymbolHeaderIDs;
+static llvm::DenseMap<int, std::pair<llvm::StringRef, llvm::StringRef>>
+ *SymbolNames;
----------------
hokein wrote:
> int => unsigned.
>
> Any reason to change it to a map? My understanding is that SymbolNames is a
> mapping from the SymbolID => {Scope, Name}, it should not be affected if we
> add multiple-header symbols.
Right. I am not sure what I was thinking. Possibly I was thinking that symbol
IDs won't be continuous anymore after this change, but they actually are.
Thanks!
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:58
+ int SymIndex = NextAvailSymIndex;
+ if (NSSymbolMap *NSSymbols = NamespaceSymbols->lookup(NS)) {
+ auto It = NSSymbols->find(Name);
----------------
hokein wrote:
> Given the fact that multiple-header symbols are grouped in the .inc file, we
> could simplify the code of checking a new symbol by looking at the last
> available SymIndex:
>
> ```
> if (NextAvailSymIndex > 0 && SymbolNames[NextAvailSymIndex-1].first == NS &&
> SymbolNames[NextAvailSymIndex-1].second == Name) {
> // this is not a new symbol.
> }
> ```
I know this is easier in terms of code, but this heavily relies on the order in
the generated files. I would prefer to keep the library and the generator as
decoupled as possible, even if it means slightly more complex code here.
Overall, it seems more future-proof in case of unrelated generator changes
(bugs?) that might change the order.
================
Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:103
+llvm::StringRef Symbol::scope() const {
+ auto It = SymbolNames->find(ID);
+ if (It != SymbolNames->end()) {
----------------
hokein wrote:
> The `ID` field is guaranteed to be valid, so no need to do the sanity check.
This is all gone now, since the code is back to the array version of
SymbolNames.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142092/new/
https://reviews.llvm.org/D142092
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
