[PATCH] D142092: [include-mapping] Allow multiple headers for the same symbol. Choose the first header of available ones.

[Viktoriia Bakalova via Phabricator via cfe-commits]

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