[PATCH] D86142: [LLD] Search archives for non-tentative defintions.

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

The general idea makes sense to me.
I have a set of mostly minor comments here and there.



================
Comment at: lld/ELF/InputFiles.cpp:1206
+template <class ELFT>
+bool isUncommonDef(MemoryBufferRef mb, StringRef symName,
+                          StringRef archiveName) {
----------------
You can probably make this `static` since you only call it from one place that I can see.
Same with the function below.


================
Comment at: lld/ELF/InputFiles.cpp:1212
+  for (auto sym : obj->template getGlobalELFSyms<ELFT>()) {
+    Expected<StringRef> name = sym.getName(stringtable);
+    if (name && name.get().equals(symName))
----------------
Question:
Why do you use `Expected<>` here?
Won't `getName()` always return a valid `StringRef`?

Since you have this here I assume you have come across a situation where you need it.


================
Comment at: lld/ELF/InputFiles.cpp:1214
+    if (name && name.get().equals(symName))
+      return sym.isDefined() && !sym.isCommon();
+  }
----------------
I think that by definition we cannot have both `sym.isDefined()` and `sym.isCommon()` to be both true at the same time. 
```
  bool isCommon() const { return symbolKind == CommonKind; }
  bool isDefined() const { return symbolKind == DefinedKind; }
```
You might be able to just use
```
return sym.isDefined();
```


================
Comment at: lld/ELF/Symbols.cpp:705
+        replace(other);
+        type = ty;
+        other.fetch();
----------------
You probably don't need to save and restore the type here.
You know based on the guard that the symbol is `isObject()` so you can just use:
```
replace(other);
type = llvm::ELF::STT_OBJECT;
```


================
Comment at: lld/test/ELF/common-archive-lookup.s:31
+  .text
+  .abiversion 2
+  .global foo
----------------
nit:
Do you need the ABI version? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86142/new/

https://reviews.llvm.org/D86142