Use more early returns in SymbolTable.cpp.

[ruiu]

Summary

I think if statements that end with return is easier to read than
cascaded if-else-if-else-if statements because it makes clear that
execution of a function terminates there. This patch also adds more
blank lines to separate code blocks to meaningful pieces.

[ruiu]

• reduces number of checkSymbolTypes function calls.

[sbc100]

This is going to conflict pretty badly with https://reviews.llvm.org/D43496... which are we going to land first? :)

[ruiu]

• fix typo

[ruiu]

Please go first.

[ruiu]

• rebased

[sbc100]

Nice!

lld/wasm/SymbolTable.h

 // add*() functions, which are called by input files as they are parsed.
 // There is one add* function per symbol type.
 class SymbolTable {
 public:
   void addFile(InputFile *File);
 
   std::vector<ObjFile *> ObjectFiles;
 
-  void reportDuplicate(Symbol *Existing, InputFile *NewFile);
   void reportRemainingUndefines();
 
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
   Symbol *find(StringRef Name);
   ObjFile *findComdat(StringRef Name) const;
 
   Symbol *addDefined(bool IsFunction, StringRef Name, uint32_t Flags,
                      InputFile *F, InputChunk *Chunk = nullptr,

lld/wasm/SymbolTable.cpp

std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {
   Symbol *&Sym = SymMap[CachedHashStringRef(Name)];
   if (Sym)
     return {Sym, false};
   Sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
   SymVector.emplace_back(Sym);
   return {Sym, true};
 }
 
-void SymbolTable::reportDuplicate(Symbol *Existing, InputFile *NewFile) {
-  error("duplicate symbol: " + toString(*Existing) + "\n>>> defined in " +
-        toString(Existing->getFile()) + "\n>>> defined in " +
-        toString(NewFile));
-}
-
 // Check the type of new symbol matches that of the symbol is replacing.
 // For functions this can also involve verifying that the signatures match.
 static void checkSymbolTypes(const Symbol &Existing, const InputFile &F,
                              bool NewIsFunction, const WasmSignature *NewSig) {
   if (Existing.isLazy())
     return;
 
   // First check the symbol types match (i.e. either both are function

 Symbol *SymbolTable::addDefined(bool IsFunction, StringRef Name, uint32_t Flags,
                                 InputFile *F, InputChunk *Chunk,
                                 uint32_t Address) {
   if (IsFunction)
     DEBUG(dbgs() << "addDefined: func:" << Name << "\n");
   else
     DEBUG(dbgs() << "addDefined: global:" << Name << " addr:" << Address
                  << "\n");
+
   Symbol *S;
   bool WasInserted;
-  bool Replace = false;
-  bool CheckTypes = false;
-
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted) {
-    Replace = true;
-  } else if (S->isLazy()) {
-    // Existing symbol is lazy. Replace it without checking types since
-    // lazy symbols don't have any type information.
-    DEBUG(dbgs() << "replacing existing lazy symbol: " << Name << "\n");
-    Replace = true;
-  } else if (!S->isDefined()) {
-    // Existing symbol is undefined: replace it, while check types.
-    DEBUG(dbgs() << "resolving existing undefined symbol: " << Name << "\n");
-    Replace = true;
-    CheckTypes = true;
-  } else if ((Flags & WASM_SYMBOL_BINDING_MASK) == WASM_SYMBOL_BINDING_WEAK) {
-    // the new symbol is weak we can ignore it
-    DEBUG(dbgs() << "existing symbol takes precedence\n");
-    CheckTypes = true;
-  } else if (S->isWeak()) {
-    // the existing symbol is, so we replace it
-    DEBUG(dbgs() << "replacing existing weak symbol\n");
-    Replace = true;
-    CheckTypes = true;
-  } else {
-    // neither symbol is week. They conflict.
-    reportDuplicate(S, F);
-  }
 
-  if (Replace) {
-    if (CheckTypes)
-      checkSymbolTypes(*S, *F, IsFunction, Chunk);
+  auto Replace = [&] {
     if (IsFunction)
       replaceSymbol<DefinedFunction>(S, Name, Flags, F,
                                      cast<InputFunction>(Chunk));
     else
       replaceSymbol<DefinedGlobal>(S, Name, Flags, F, cast<InputSegment>(Chunk),
                                    Address);
+  };
+
+  // If there's no existing symbol, just insert a new one.
+  if (WasInserted) {
+    Replace();
+    return S;
+  }
+
+  // If existing symbol is lazy, replace it without checking types since
+  // lazy symbols don't have any type information.
+  if (S->isLazy()) {
+    DEBUG(dbgs() << "replacing existing lazy symbol: " << Name << "\n");
+    Replace();
+    return S;
+  }
+
+  // Now we have two wasm symbols, and all wasm symbols that have the same
+  // symbol name must have the same type, even if they are undefined. This
+  // is different from ELF because symbol types are not that significant
+  // in ELF, and undefined symbols in ELF don't have type in the first place.
+  checkSymbolTypes(*S, *F, IsFunction, Chunk);
+
+  // If existing symbol is undefined, replace it while checking types.
+  if (!S->isDefined()) {
+    DEBUG(dbgs() << "resolving existing undefined symbol: " << Name << "\n");
+    Replace();
+    return S;
   }
+
+  // Now we have two defined symbols. If the new one is weak, we can ignore it.
+  if ((Flags & WASM_SYMBOL_BINDING_MASK) == WASM_SYMBOL_BINDING_WEAK) {
+    DEBUG(dbgs() << "existing symbol takes precedence\n");
+    return S;
+  }
+
+  // If the existing symbol is weak, we should replace it.
+  if (S->isWeak()) {
+    DEBUG(dbgs() << "replacing existing weak symbol\n");
+    Replace();
+    return S;
+  }
+
+  // Neither symbol is week. They conflict.
+  error("duplicate symbol: " + toString(*S) + "\n>>> defined in " +
+        toString(S->getFile()) + "\n>>> defined in " + toString(F));
   return S;
 }
 
 Symbol *SymbolTable::addUndefined(StringRef Name, Symbol::Kind Kind,
                                   uint32_t Flags, InputFile *F,
                                   const WasmSignature *Type) {
   DEBUG(dbgs() << "addUndefined: " << Name << "\n");
+
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
+
   bool IsFunction = Kind == Symbol::UndefinedFunctionKind;
+
   if (WasInserted) {
     if (IsFunction)
       replaceSymbol<UndefinedFunction>(S, Name, Flags, F, Type);
     else
       replaceSymbol<UndefinedGlobal>(S, Name, Flags, F);
-  } else if (auto *LazySym = dyn_cast<LazySymbol>(S)) {
+    return S;
+  }
+
+  if (auto *Lazy = dyn_cast<LazySymbol>(S)) {
     DEBUG(dbgs() << "resolved by existing lazy\n");
-    auto *AF = cast<ArchiveFile>(LazySym->getFile());
-    AF->addMember(&LazySym->getArchiveSymbol());
-  } else if (S->isDefined()) {
+    cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
+    return S;
+  }
+
+  if (S->isDefined()) {
     DEBUG(dbgs() << "resolved by existing\n");
     checkSymbolTypes(*S, *F, IsFunction, Type);
   }
   return S;
 }
 
 void SymbolTable::addLazy(ArchiveFile *F, const Archive::Symbol *Sym) {
   DEBUG(dbgs() << "addLazy: " << Sym->getName() << "\n");
-  StringRef Name = Sym->getName();
+
   Symbol *S;
   bool WasInserted;
-  std::tie(S, WasInserted) = insert(Name);
+  std::tie(S, WasInserted) = insert(Sym->getName());
+
   if (WasInserted) {
-    replaceSymbol<LazySymbol>(S, Name, F, *Sym);
-  } else if (S->isUndefined()) {
-    // There is an existing undefined symbol.  The can load from the
-    // archive.
+    replaceSymbol<LazySymbol>(S, Sym->getName(), F, *Sym);
+    return;
+  }
+
+  // If there is an existing undefined symbol, load a new one from the archive.
+  if (S->isUndefined()) {
     DEBUG(dbgs() << "replacing existing undefined\n");
     F->addMember(Sym);
   }
 }
 
 bool SymbolTable::addComdat(StringRef Name, ObjFile *F) {
   DEBUG(dbgs() << "addComdat: " << Name << "\n");
   ObjFile *&File = ComdatMap[CachedHashStringRef(Name)];