[PATCH] D111101: [lld][WebAssembly] Remove redundant check for undefined global (NFC)

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lld/wasm/Relocations.cpp:45
-    if (g->importName)
-      return true;
   return config->allowUndefinedSymbols.count(sym->getName()) != 0;
----------------
sbc100 wrote:
> aheejin wrote:
> > sbc100 wrote:
> > > Actually there was a recent refactor that made `importName` a (optional) member of the Symbol superclass.. so I think this can be written as:
> > > 
> > > ```
> > > if (sym->importName)
> > >   return true;
> > > if (isa<UndefinedFunction>(sym) && config->importUndefined)
> > >   return true;
> > > ```
> > > 
> > > But if you would rather land this as is that is fine too.
> > > if (isa<UndefinedFunction>(sym) && config->importUndefined)
> > 
> > Isn't this `||` in the current code?
> > 
> > Also, if we return `true` unconditionally when `sym->importName` is defined, I think we return `true` for data or tables. Is that what you intended?
> Yes, any symbol with an explicit `importName` should always be imported and never have "undefined symbol" reported about it.
Which one is correct?
```
if (isa<UndefinedFunction>(sym) && config->importUndefined)
```
vs.
```
if (isa<UndefinedFunction>(sym) || config->importUndefined)
```
(The current code is like the latter and you suggested the former)

Also, `config->importUndefined` doesn't affect `UndefinedGlobal`, or `UndefinedTag` I am going to add?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111101