[PATCH] D91884: clang+lld: Improve clang+ld.darwinnew.lld interaction, pass -demangle

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks!



================
Comment at: clang/lib/Driver/ToolChain.cpp:606
+        if (UseLinker == "lld" || UseLinker == "lld.darwinnew")
+          *LinkerIsLLD = true;
       return LinkerPath;
----------------
hans wrote:
> It seems nicer for the caller if *LinkerIsLLD is always set to either true or false, so the caller doesn't have to initialize it.
> 
> For example:
> 
> ```
> if (LinkerIsLLD)
>   *LinkerIsLLD = (UseLinker == "lld" || UseLinker == "lld.darwinnew");
> ```
If we guarantee to set it to false, we need to initialize it on all paths (ie set it to false at the start of the function). But done.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:696
 
-  const char *Exec = Args.MakeArgString(getToolChain().GetLinkerPath());
   std::unique_ptr<Command> Cmd = std::make_unique<Command>(
----------------
hans wrote:
> Why is this removed?
I moved it up to line 540, since I need the bool that this now sets up there.


================
Comment at: lld/Common/Strings.cpp:29
   // result for a C symbol that happens to match a mangled type name.
-  if (!name.startswith("_Z"))
+  if (!name.startswith("_Z") && !name.startswith("__Z") &&
+      !name.startswith("___Z") && !name.startswith("____Z"))
----------------
hans wrote:
> Does the comment above need updating now that this is checking not just for "_Z", but also multiple leading underscores?
> Actually I found the comment pretty confusing in its current state too.
> The idea here is to only call the demangler for C++ symbols, right? Maybe the comment could just say that.
Removed some bits, added some bits, added an example.


================
Comment at: lld/MachO/Options.td:1109
      Group<grp_undocumented>;
 def demangle : Flag<["-"], "demangle">,
+     HelpText<"Demangle symbol names in diagnostics">;
----------------
hans wrote:
> All options seem to belong to a Group and also be ordered in the .td file according to group. Now that it's no longer in grp_undocumented, maybe it should move into another group?
Options without explicit end up in "OPTIONS:" in `lld -flavor darwinnew --help`, which seems the correct group for this flag. Do you think it should be in a different group? A new group?


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

https://reviews.llvm.org/D91884