[PATCH] D47540: [lld] Enable Visual Studio compatible output

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I reviewed this patch, but I'm not convinced that we really want this. You are changing the ELF linker, but how many people are using our ELF linker on Visual Studio? Did you try to talk with Microsoft to let them make a change for you instead of making a change to us? I'm not trying to reject this, but adding a new feature and a new command line flag always needs a fairly strong justification, so please explain how much useful this is.



================
Comment at: ELF/Driver.cpp:76-78
+  errorHandler().LogName = errorHandler().VSDiagnostics
+                               ? sys::path::stem(sys::path::filename(Args[0]))
+                               : Args[0];
----------------
This should be moved to ErrorHandlers.cpp.


================
Comment at: ELF/Driver.cpp:682-683
 
+  errorHandler().VSDiagnostics =
+      Args.hasArg(OPT_visual_studio_diagnostics_format);
+
----------------
This is perhaps too late. This should be processed at the same timing as --color-diagnostics.


================
Comment at: ELF/DriverUtils.cpp:134
+
+  // remove executable name from the argument vectorso it is not later
+  // interpreted as an input object file
----------------
Why do you need to add this code? If it is working now, you don't need to do this.


================
Comment at: ELF/InputFiles.cpp:88
+    return Path.str() + " " + "(" + std::to_string(Line) + ")";
+  } else {
+    std::string Filename = path::filename(Path);
----------------
No else after return.


================
Comment at: ELF/Relocations.cpp:638
       Config->NoinhibitExec) {
-    warn(Msg);
+    warn(Msg, Src);
     return false;
----------------
You include `Src` in an error message and also pass `Src` to the function. As a result, I believe the same string is printed out more than once. That would look pretty odd.


================
Comment at: include/lld/Common/ErrorHandler.h:75-76
+}
+inline LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &Msg,
+                                          const Twine &Origin = "") {
+  errorHandler().fatal(Msg, Origin);
----------------
I don't like a function with a default argument as it is too easy to mess up. People casually add additional parameters to existing functions, and the result would be a function that has tons of parameters. Please avoid doing this if possible.


https://reviews.llvm.org/D47540