Wed, Oct 3
@lantictac The following works in the test:
RUN: lld-link %S/a.obj /entry:"1 "'"'hello'"'" 2"
Outputs as expected:
lld-link: error: <root>: undefined symbol: 1 "hello" 2
Tue, Oct 2
Thanks for the clarification. Is there any way to manually close the review?
Ahh I see. You need to say "Differential Revision:
https://reviews.llvm.org/D50404" in the commit message. just the URL is
not sufficient to trigger the magic auto-close.
I committed this as r342894 but for some reason the review didn't complete. Apologies, the last LLVM commits I did were pre-phabricator so it seems I messed up somewhere. Any pointers appreciated.
It would be great if we could land this soon-ish. I have a lot of people complaining that lld-link rejects /DEBUG:FASTLINK when using the VS extension. Is there any work left to be done or can we land this?
Fri, Sep 28
Wed, Sep 26
LG (but please wait for the code owner).
Thu, Sep 20
With overloaded operators we can write something like
error("relocation " + toString(Type) + " cannot refer to absolute symbol: " + toString(Sym) + definedAt(toObjLoc(Sym.File)) + referencedBy(S.getSymLoc(Sym, RelOff)));
How would you prefer it to look like?
I don't insist on the current style, but I believe it is important to be discussed prior to rewriting the code because the decision will influence the design directly.
Updated patch with ruiu's suggestions. Also added a test for the warning emitted when an unknown /debugtype option is used.
Looks much better!
Updated patch based on suggestions from ruiu
Operator overriding can always be rewritten as function calls, so instead of operator+(X), you can always define something like add(X)?
Moved/inlined parsing and diagnostic code into the specialized parse functions to (hopefully) simplify the calls in LinkerDriver::link().
Wed, Sep 19
I haven't read Diagnostics.cpp to focus on the ABI itself, but it looks like this approach is better than https://reviews.llvm.org/D47540, though this patch seems a bit too abstracted and too object-oriented to my taste. Can you add comment to the code? Also, please avoid operator overriding and eliminate Diag class (you can do that because all you need is a boolean flag indicating whether VS output is wanted or not.)
Sep 13 2018
It needs a test, but I'll write that since the test should be small.
If you have a commit access, you can do that yourself, but since you don't have one, I'll do that for you.
If this looks fine, could you please merge it in? I’m not sure how the merge process is handled for LLVM.
Sep 12 2018
Sep 10 2018
I think I don't understand the use case of the feature yet.
Specify std::string over auto and change message to Msg.
This seems like a new feature proposal, and we haven't discussed this before. It's not clear to me why you have to do this inside the linker rather than a post-processing tool. Could you please elaborate about why you want to add a new option?
Sep 8 2018
Sep 7 2018
Address review comments by inlining errorOrWarnMultiple.
I have experimented with this implementation and I agree that separating the diagnostic location functions into a distinct library is a good structured approach. I think this would be a reasonable and more explicit support for Visual Studio Diagnostics than D47540 which really just shoehorned in support.
Sep 5 2018
I made a comment in https://bugs.llvm.org/show_bug.cgi?id=38784
You'll wanna upload the patch with full context (http://llvm.org/docs/Phabricator.html).
Renamed DebugGeneration enum (name was based on MSDN description of /debug argument) to DebugKind. Flattened the argument handling code to be a little clearer.