[PATCH] D91884: clang+lld: Improve clang+ld.darwinnew.lld interaction, pass -demangle
Nico Weber via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 23 05:50:53 PST 2020
thakis added a comment.
Comment at: clang/lib/Driver/ToolChain.cpp:606
+ if (UseLinker == "lld" || UseLinker == "lld.darwinnew")
+ *LinkerIsLLD = true;
> 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>(
> 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"))
> 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
def demangle : Flag<["-"], "demangle">,
+ HelpText<"Demangle symbol names in diagnostics">;
> 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
More information about the llvm-commits