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

[Andy Wingo via Phabricator via llvm-commits]

wingo added a comment.

I see that this is marked changes requested, but I don't understand what changes -- is it InputItem?



================
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);
 }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94677