[PATCH] D94677: [lld][WebAssembly] Common superclass for input globals/events/tables

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/wasm/Writer.cpp:213
 static void setGlobalPtr(DefinedGlobal *g, uint64_t memoryPtr) {
-  if (config->is64.getValueOr(false)) {
-    assert(g->global->global.InitExpr.Opcode == WASM_OPCODE_I64_CONST);
-    g->global->global.InitExpr.Value.Int64 = memoryPtr;
-  } else {
-    assert(g->global->global.InitExpr.Opcode == WASM_OPCODE_I32_CONST);
-    g->global->global.InitExpr.Value.Int32 = memoryPtr;
-  }
+  g->global->setPointerValue(memoryPtr);
 }
----------------
wingo wrote:
> sbc100 wrote:
> > Is moving this code into a method a separate refactor, or essential to the PR?  (Just wondering, not asking you split it out necessarily).
> > 
> > Why make this into its own method?  Given that it only has a single call site.   I'm mostly curious what your thinking is, not opposed to this.
> > 
> > Do you think its still worth keeping this function now now that its just one line?  
> Paging this back in -- I think this was because the different InputGlobal, InputEvent etc had different styles with regards to what was public instance state and what had to be accessed via methods.  To harmonize these things, now that they are all in one file, I made the InitExpr be private and added the method accessor.  & Of course no, this setGlobalPointer method is certainly no longer needed at this point.  WDYT?
I'll leave to you..  if you think this one-line method makes the code below more readable feel free to leave it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94677