[PATCH] D57006: [llvm-objcopy] [COFF] Update symbol indices in weak externals

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:83-85
+        return make_error<StringError>("Symbol " + Sym.Name +
+                                           " is missing its weak target",
+                                       object_error::invalid_symbol_index);
----------------
mstorsjo wrote:
> jhenderson wrote:
> > mstorsjo wrote:
> > > jhenderson wrote:
> > > > Ditto. In this case, you can then take advantage of the printf-style overload to avoid the string concatenation here.
> > > > 
> > > > Also putting some quotes around the symbol name might be good, especially if it's theoretically possible to have spaces in your symbol names in COFF.
> > > With printf-style formatting, I'd either need to make the StringRef a null terminated string, or use the `%.*s` formatting syntax with `Sym.Name.size(), Sym.Name.data()` or something similar, which kinda negates any convenience of printf-style formatting strings.
> > > 
> > > About quotes around the symbol names - oh, right, yes. It seems I've slipped in a few more of these before as well, I can push an NFC patch that fixes those, the next time I push something.
> > You could use StringRef::str() to avoid this.
> > 
> > Updating the other errors later would be good.
> Wouldn't that need something like `Sym.Name.str().c_str()` then? That's also a bit clunky but slightly leaner than the `%.*s` form.
Possibly. I can't remember exactly how the internals work. I think I'd prefer it to the current form, because it keeps the pattern simpler.


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

https://reviews.llvm.org/D57006