Should there be some tests under llvm/test/FileCheck as well? Perhaps in numeric-expressions.txt?

In D77227#1955859, @probinson wrote:

In D77227#1955808, @jdenny wrote:

I agree we don't want these diagnostics to examine FileCheck prefixes within RUN lines. I was just observing that the rule as I formulated it above would be sufficient for that purpose, so we wouldn't need a separate constraint for RUN lines.

Ah, so I misunderstood what you meant by "the above rule" sorry about that! If we require a comment marker to precede the directive, then we're good (until some clever language decides to use '=' as their comment delimiter).

Address last set of comments

Use readelf instead of readobj

In D75631#1943963, @davidb wrote:

Test change looks good (address lookup rather than by name), but it makes the objdump change completely irrelevant to this now, so I'd revert the objdump change.

Incorporate DISASM improvements from D75631

Cross-check address of call targets in lld/test/ELF/pre_init_fini_array.s against section address with --syms

Remove assert

In D75631#1941289, @davidb wrote:

This still needed after D76739?

LGTM. This is what I think is causing tools/llvm-objdump/X86/disassemble-functions.test to fail in some settings (see my comment https://reviews.llvm.org/D75631#1940245)

Fix source of variability in test

In D75631#1913554, @echristo wrote:

The change looks good to me regardless, but i'd like to understand the issue causing the variability in behaviour before giving formal approval, as non-determinism like this is bad, and might indicate something like an unitialised variable somewhere or some other bug.

I will also be nervous to approve this change, without understanding how the test fails.

Agreed.

-eric

In D75631#1909084, @jhenderson wrote:

@MaskRay just made a change to some disassembly output (see D75713). I'm not sure if it's going to impact this change or not, but you should check. Also, could you clarify what you mean by "sometimes disassembled" please? Why does it change?

Rebase and adapt after D75713

Ping?

Committed as 23294fb9ca142f9982dcfac1044b589de0baf481

• Fix typo in doc
• remove sneaky literal for LineNumber in unit test

Address unit tests review comments

Address all review comments but the location information

Address all review comments

Add still missing error checks...

Add missing error checks

Address all review comments but the substring match when expecting errors

Address review comments

Address review comments

Committed as 10f1592ecb34bdbf2d7c62736aaf987df2fdd511

In D68779#1811800, @thopre wrote:

Ping @cmatthews ?

• Address unit tests review comments
• fix casing of first letter in an error message

Introduce StringMatcher.empty()

In D72517#1823830, @evgeny777 wrote:

I think that adding extra flag to GlobPattern::create was better idea. Just set it to false by default, so that GlobPattern will still act as glob matcher for all other users:

static static Expected<GlobPattern> create(StringRef Pat, bool IgnoreMetaChars = false);

Than add extra constructor to StringMatcher to which you'll pass GlobPattern and use it from InputSectionDescription constructor.

In D72517#1814982, @MaskRay wrote:

allows to fix a failure in ELF/linkerscript/filename-spec.s when the path contain a @ which the lexer consider as a special characters and thus break up a filename containing it.

Which one?

• Make exact match at StringMatcher level
• Add possibility to add new pattern to a StringMatcher

Ping @cmatthews ?

Need to add unit tests

Need to add unit tests

Rebase

Complete unit testing of changes in the patch

Is everybody happy with the constness changes to the ExpressionFormat operators and the new unit tests?

In D68054#1806142, @nlguillemot wrote:

In D68054#1801066, @thopre wrote:

How about the following commit message:

(...)

I would suggest an amendment to this part:

To force developers to be mindful of this aspect, an assert is added to match() to check
that the regex is valid and an new idiom is created as follows for
cases where the pattern is known to be valid:

As-is, this patch doesn't assert inside match(), since this makes the API more backwards compatible. The wording of the commit message should be updated to match this.

It was originally a bit tricky to track down and update all users of the API, but the monorepo makes that a lot easier. If we brought back the older version of the commit that *did* assert inside match(), and we updated all affected users of the API (eg: clang) before committing, I wouldn't be opposed.

My apologies but I still think there's changes needed on this patch. I had changed the status earlier but I presume updating the patch did reset the status to "needs review". I haven't covered all the API changes in the unittests and I'd like to address the comment from Paul about the lack of clarity of which variables have implicit format conflict. I'm happy to deal with the latter in a separate patch if people are eager to have this change (I'm personally looking forward to it being committed as well) but I want to at least finish the unit testing.

Use EXPECT_THAT_ERROR when testing for an error in unittest

Do not rename FileCheckPatternContext

Rebase

Rebase

Address latest round of review comments

Rebase