[PATCH] D77187: [lld][WebAssembly] Add initial support for -Map/--print-map

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/test/wasm/map-file.s:3
+// RUN: wasm-ld %t1.o -o %t -M | FileCheck -strict-whitespace %s
+// RUN: wasm-ld %t1.o -o %t -print-map | FileCheck -strict-whitespace %s
+// RUN: wasm-ld %t1.o -o %t -Map=%t.map
----------------
sbc100 wrote:
> MaskRay wrote:
> > --match-full-lines --strip-whitespace
> > 
> > --strip-whitespace alone still ignores leading or trailing spaces.
> > 
> > See `test/ELF/map-file-64bit.s` for an example
> Sure thing.. I was copying `lld/test/ELF/map-file.s` which just uses `-strict-whitespace`.
> 
> At least it did until today when you changed it in cece7af58682a2122b108d7af270a31043ac1825 :)  
> 
> Any reason you also changed all the comments from `//` to `#` in that change?   I think we should try to be consistent across all the `.s` tests in lld so maybe we change all of them at once?  (Assuming we prefer the hash mark).
> 
> Also, I looks like you didn't send it for review (or at least no here on phabicator)?  Was that deliberate?
> 
> Sorry, I don't mean to sound picky..  happy for the feedback here.
I think `lld/test/ELF/map-file.s` should have been fixed earlier before you copied it to wasm.

About `//` -> `#`, the description of that commit mentioned that since I had to change so many lines (adding leading spaces), just change the comments to use the more common way.

For other tests, it might just create churn if we do so. So I would not want to do that unless there is just motivation and I have to change a majority of lines (then optimizing for diff no longer counts)

> Also, I looks like you didn't send it for review (or at least no here on phabicator)? Was that deliberate?
>
> Sorry, I don't mean to sound picky.. happy for the feedback here.

Yes. It is NFC and likely community consensus plus I have made sufficient changes to lld/ELF. I probably would not want to do that if you did not copy it here.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77187