[PATCH] D61855: Simplify SymbolTable::add{Defined,Undefined,...} functions.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu marked 4 inline comments as done.
ruiu added a comment.

This is the same as the previous patch. If there's no big issue in this patch, I'd like to commit after making changes as you guys pointed out.



================
Comment at: lld/ELF/Driver.cpp:1379
+  Undefined Sym(nullptr, Name, STB_GLOBAL, STV_DEFAULT, 0);
+  return Symtab->addUndefined<ELFT>(&Sym);
 }
----------------
grimar wrote:
> What was confusing for me when I saw this in your patch is that `Sym` is a pointer.
> I.e. I was wondering can `addUndefined` store it somewhere inside or not. Should it be a reference may be?
> 
> And then you should be able to:
> 
> ```
> return Symtab->addUndefined<ELFT>({nullptr, Name, STB_GLOBAL, STV_DEFAULT, 0)});
> ```
I could indeed make it a const reference. That's perhaps a matter of personal taste, but since I don't feel strongly about . that, I'll change the parameter type as you suggested.


================
Comment at: lld/ELF/SymbolTable.cpp:121
+
+    Old->SymbolKind = Symbol::PlaceholderKind;
+    Old->VersionId = Config->DefaultSymbolVersion;
----------------
MaskRay wrote:
> It seems after every `insert()` call, if the `IsNew` branch is taken, these member variables are initialized then get overridden by `replaceSymbol(Old, New)`. Is there some way to prevent repeated assignments?
Good point, and I tried that before submitting this patch for review. It turned out that doing that isn't as easy as it seems. There may be some good way to do that, though. I'd want to keep it as-is for now.


================
Comment at: lld/ELF/Symbols.h:368
 
-template <typename T, typename... ArgT>
-void replaceSymbol(Symbol *S, ArgT &&... Arg) {
+template <typename T> void replaceSymbol(Symbol *Sym, T *New) {
   using llvm::ELF::STT_TLS;
----------------
MaskRay wrote:
> Agree with George. `T *New` -> `const T &New`
Will do.


================
Comment at: lld/ELF/Symbols.h:384
   // as a TLS symbol in one file and as a non-TLS symbol in other file.
-  bool TlsMismatch = (Sym.Type == STT_TLS && S->Type != STT_TLS) ||
-                     (Sym.Type != STT_TLS && S->Type == STT_TLS);
+  if (Sym->SymbolKind != Symbol::PlaceholderKind && !Sym->isLazy() &&
+      !New->isLazy()) {
----------------
MaskRay wrote:
> `Sym->isLazy()` can be deleted because an `isLazy()` symbol is `STT_NOTYPE`.
We actually need this because if we delete `Sym->isLazy()` and if we have a lazy symbol and a TLS symbol, we'll end up comparing STT_NOTYPE and STT_TLS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61855