[PATCH] D42475: [ELF] Add warnings for various symbols that cannot be ordered

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: ELF/Writer.cpp:1046
+      auto OrderEntry = SymbolOrder.find(Sym->getName());
+      if (Config->WarnSymbolOrdering && OrderEntry != SymbolOrder.end()) {
+        if (Sym->isUndefined())
----------------
grimar wrote:
> Looks you can use early continue:
> 
> ```
> if (OrderEntry == SymbolOrder.end())
>   continue;
> OrderEntry->second.second = true;
> ```
> 
> With that code below seems simplifies to:
> 
> ```
> int &Priority = SectionOrder[D->Section];
> Priority = std::min(Priority, OrderEntry->second.first);
> ```
Thanks, you're right. I think the problem I thought was that there would not be entries in SectionOrder for all sections in the end, if we stopped early, but actually, that doesn't matter, because we use lookup() on this map eventually, so a missing value is the same as 0.


================
Comment at: ELF/Writer.cpp:1073
+  if (Config->WarnSymbolOrdering)
+    for (auto V : SymbolOrder) {
+      bool Used = V.second.second;
----------------
grimar wrote:
> We usually inline such short conditions I believe:
> 
> ```
>   if (Config->WarnSymbolOrdering)
>     for (auto V : SymbolOrder)
>       if (!V.second.second)
>         warn("symbol ordering file: no symbol '" + V.first +
>              "' found in input");
> ```
> 
> Also `V` name usually stands for Vector, though it is not the case here. I would rename.
> 
> Did you consider to use `MapVector` for SymbolOrder to iterate in stable insertion order here ?
> 
No, I hadn't considered `MapVector`, but I think it is a good idea. Thanks for suggesting it.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42475