[PATCH] D63901: [WebAssembly] Added visibility and ident directives to WasmAsmParser.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 3 inline comments as done.
aardappel added inline comments.


================
Comment at: lib/MC/MCParser/WasmAsmParser.cpp:233
+    MCSymbolAttr Attr = StringSwitch<MCSymbolAttr>(Directive)
+      .Case(".weak", MCSA_Weak)
+      .Case(".local", MCSA_Local)
----------------
sbc100 wrote:
> dschuff wrote:
> > @sbc100 do we support exactly the same set of visibility types as ELF? I think the answer is no, right? In that case it probably also answers the question of whether we can share code with ELF too.
> Technically we don't support "protected" yet, but there is no reason why we shouldn't in the future.   Even though we don't support it its not clear to me if we should generate an error here, or perhaps in EmitSymbolAttribute?
I will comment out "protected" for now, which will make it error.


================
Comment at: test/MC/WebAssembly/basic-assembly.s:91
     .section	.rodata..L.str,"",@
+    .hidden     .L.str
 .L.str:
----------------
sbc100 wrote:
> Align second column with line above?
File had mixed tabs and spaces, which makes it look different in code review.. removing tabs.


================
Comment at: test/MC/WebAssembly/basic-assembly.s:179
 # CHECK:	    .section	.rodata..L.str,"",@
+# CHECK-NEXT:   .hidden     .L.str
 # CHECK-NEXT:.L.str:
----------------
sbc100 wrote:
> Indentation looks great than directives below?
I'll make indentation in this file more consistent..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63901