This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][MC] Fix leak of std::string members in MCSymbolWasm
ClosedPublic

Authored by sbc100 on Apr 6 2020, 8:49 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Apr 6 2020, 8:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:49 PM
sbc100 updated this revision to Diff 255578.Apr 6 2020, 8:51 PM
  • revert
sunfish accepted this revision.Apr 7 2020, 10:08 AM
sunfish added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
50

Can this function be private in WebAssemblyAsmPrinter?

This revision is now accepted and ready to land.Apr 7 2020, 10:08 AM
sbc100 updated this revision to Diff 255723.Apr 7 2020, 10:38 AM
sbc100 marked an inline comment as done.

feedback

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.
llvm/include/llvm/MC/MCSymbolWasm.h
26–28

Do these need to be std::strings, or could they just be StringRefs?

It looks like they're only set/get in this class, never modified by it - StringRefs might be more suitable? (does the empty string need to be a separate state from "none"/null - I'd probably still go with Optional<StringRef> then - though opinions certainly might differ there - in that case (where opinions differ & you prefer std::string* over Optional<StringRef> here) I'd at least make them const std::string*))

sbc100 marked an inline comment as done.Apr 7 2020, 11:57 AM
sbc100 added inline comments.
llvm/include/llvm/MC/MCSymbolWasm.h
26–28

Hm, you are correct. I'm not sure why I didn't go with Optional<StringRef>. I'll update.