[PATCH] D61811: [WebAssembly] Refactor synthetic sections and relocation processing. NFC.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking good. I didn't read each line that seems to be just a code move, though.



================
Comment at: lld/wasm/Symbols.h:20-21
 
+extern const char *DefaultModule;
+extern const char *FunctionTableName;
+
----------------
sbc100 wrote:
> ruiu wrote:
> > It would be nice to explain them as comment.
> Done.
> 
> These are shared constants.. can you think of a better place to store them?  Maybe Config?  
I think just global variables are fine. Config are mainly for command line parameters.


================
Comment at: lld/wasm/SyntheticSections.cpp:509
+void ProducersSection::writeBody() {
+
+  auto &OS = BodyOutputStream;
----------------
nit: remove the empty line.


================
Comment at: lld/wasm/SyntheticSections.h:318
+
+extern InStruct In;
+
----------------
sbc100 wrote:
> I was wondering about the naming of this struct and global.
> 
> In ELF I think "In" might mean "Input" since there the synthetic sections are input sections.  However we currently use OutputSections, so this name make less sense.
> 
> Perhaps I should call these "OutStruct" and "Out" or "Synth" perhaps?
Either Out and Synth are fine. It's up to you.


================
Comment at: lld/wasm/Writer.cpp:149-150
+    OutputSection *Sec = OutputSections[I];
+    if (!Sec->isNeeded())
+      continue;
+
----------------
Instead of skipping many sections that are not needed, does it make sense to add another loop to remove !isNeeded() sections from OutputSections?


================
Comment at: lld/wasm/Writer.cpp:532
+      scanRelocations(Chunk);
+    for (auto &P : File->CustomSections)
+      scanRelocations(P);
----------------
auto -> InputChunk?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61811