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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a subscriber: tlively.
sbc100 added a comment.

You can add "NFC" to the title to label this as a non-functional change.



================
Comment at: lld/wasm/InputDefinition.h:2
+//===- InputDefinition.h ----------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
I don't love the name "InputDefinition", but I don't have any great ideas for anything better.

Maybe @tlively has some thoughts?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94677