[PATCH] D97923: [WebAssembly][yaml2obj][obj2yaml] Elem sections for nonzero tables

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:937
+    W->OS << ElemKind;
+  }
+
----------------
wingo wrote:
> inline comments from @sbc100, from https://reviews.llvm.org/D92321?id=323966#inline-917851:
> 
> > > Paging this back in -- this code was not dead.  See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND.  I have restored it here.  WDYT?
> 
> > Oh I see.. WASM_ELEM_SEGMENT_HAS_ELEM_KIND is defined as ~SOME_OTHER_FLAG.. that seem odd at best.   Perhaps better write to little helper function `elemSegmentNeedsElemKind()` rather than trying to make that part of the enum?   Otherwise folks will think that `WASM_ELEM_SEGMENT_HAS_ELEM_KIND` is just another flag they can `|` together.   Either that or maybe add MASK to its name and take it out of the enum with the others.
> 
> > It looks like WASM_ELEM_SEGMENT_HAS_ELEM_KIND is basically `PASSIVE | HAS_TABLE_NUMBER` ... maybe its more clear to just write that out here?
Given that we need the logic in 4 different files, it seemed like defining the mask in a central place would be a good idea.  You're right though that as it was, you could get confused and think that `(WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER & WASM_ELEMENT_SEGMENT_HAS_ELEM_KIND) == 0`.  I changed the name therefore to `WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND`; wdyt?

There's no accounting for taste, obviously, and if you have a strong preference here, I am very happy to adapt.

I think all the other inline comments from D92321 have been addressed in this patch, fwiw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97923