LGTM.

By the way, we have some inconsistency in how cl::Prefix and cl::AlwaysPrefix options work in case of -opt=val form. The former takes only val, while the latter keeps =. Moreover, prefix grouping options in GNU's binutils, like -j in objdump, work partially as ours cl::AlwaysPrefix, because they preserve =, and partially as cl::Prefix, because they allow passing the value in a separate argument. Thus, we still cannot fully reproduce that behavior.

• Fixed grammar and formatting issues.
• Added test cases to check prefix options when their values are textually equal to another option.

In D58499#1408521, @jhenderson wrote:

LGTM.

Thanks!

In D58499#1405561, @jhenderson wrote:

What happens for ValueOptional? I want to make sure that it doesn't treat the other grouped letters after it as the option's value.

• Updated the documentation (feel free to correct my wording).
• Amended a comment in the test.

For me, the code should be, above all, reliable. You can trust it only in that case; otherwise, you never know, when and how it is going to be broken and produce an unexpected result.

Thanks! I'll wait a bit, but not very long. I believe the patch is safe because the default behavior is not changed.

Updated the test.

• Made DefaultAltIdx private.
• Extended the test.
• Addressed other review comments.

Ping.

Addressed review comments.

Thanks again, Peter!

Thank you for the explanation, Peter!

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.

@chrisjackson @arichardson @ruiu, thank you for the feedback! I am going to continue working on the patch somewhere in October. I would really appreciate any additional comments meanwhile.

If I calculate it right, your code has the same cyclomatic complexity as the original one. At the same time, it is one level deeper, taking into account the number of conditions for the particular lines.

Is it possible to add a test case?

Please see D50560 for my variant.

In D47540#1176742, @chrisjackson wrote:

While more structured, I think this approach could be even more invasive than that suggested. Do you have an example implementation?

Unfortunately, we don't run the profile test suite on Windows.

Did you consider dividing the patch into two separate changes to solve the two described issues independently?

You may want to add a test for placement new, no?

Can we try the approach where the final style of diagnostic messages is determined in the reporting module and all other code just don't care about it?
Right now, we have a bunch of functions here and there which prepare some parts of the message in a specific style. Maybe it will be more semantically clear if there is a distinct place where the resulting message is prepared while all other parts of the linker provide only basic parts of the message (like message text, source location(s), code location(s), etc.) in a structured form. In this case, we can adjust only that distinct place to add any required message styles.

Ping.

• Shortened relocateNonAllocForRelocatable();
• Made it static;
• Removed an unused argument;
• Changed an expression for Bits.

I think we need to see the other places converted, too. Reports for duplicate symbols and invalid relocations will probably require additional changes, which may lead to better design when we see the whole picture.

Ping.

Thanks!

Ping. Is there anything I can improve here?

• Add a test.

Not all toolchains call ToolChain::GetLinkerPath. For example, MSVC toolchain uses its own code:

void visualstudio::Linker::ConstructJob(...) {
...
  StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
  if (Linker.equals_lower("lld"))
    Linker = "lld-link";
...
}

In my case, I am trying to cross-compile:

> ...\clang.exe a.cpp -fuse-ld=lld -target i686-pc-linux-gnu
clang.exe: error: invalid linker name in argument '-fuse-ld=lld'