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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Writer.cpp:1039
+      warn("symbol ordering file: symbol '" + S + "' specified multiple times");
+  }
 
----------------
I would suggest to use bool here:

```
bool Inserted = SymbolOrder.insert({S, {Priority++, false}}).second;
if (Config->WarnSymbolOrdering && !Inserted)
```

as we usually trying to avoid using auto and also here type of Inserted would be pair,
though its name seems assuming it is a bool and probably looks better.


================
Comment at: ELF/Writer.cpp:1046
+      auto OrderEntry = SymbolOrder.find(Sym->getName());
+      if (Config->WarnSymbolOrdering && OrderEntry != SymbolOrder.end()) {
+        if (Sym->isUndefined())
----------------
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);
```


================
Comment at: ELF/Writer.cpp:1059
+               Sym->getName() + "' from file '" + File->getName() + "'");
+        OrderEntry->second.second = true;
+      }
----------------
Problem of that line is that it is described as "present in inputs" though set only if
`Config->WarnSymbolOrdering` is set. Though that should probably be independent.

So I suggest to move it out, like in one of above comments.


================
Comment at: ELF/Writer.cpp:1073
+  if (Config->WarnSymbolOrdering)
+    for (auto V : SymbolOrder) {
+      bool Used = V.second.second;
----------------
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 ?



Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42475