In D54769#1305396, @probinson wrote:

This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.

Thanks for working on this. I do not believe DAG overlaps should be a warning. I know from experience that they occur very frequently.

In D54764#1304573, @ABataev wrote:

I'm fine with the patch,

In D53710#1304102, @SjoerdMeijer wrote:

I'm back :-) Catching up on the last messages.... I will post a patch for review soon that adds an option -warn to FileCheck, and will do some initial plumbing for that. If I understood correctly, it looks like we think that is best. After that initial plumbing, I will then return to this patch. Is that a plan?

Update for changes earlier in patch series.

Update for changes earlier in patch series.

Update for changes earlier in patch series.

Update for changes earlier in patch series.

Update for changes earlier in patch series.

Update for changes earlier in patch series.

• Rebase, and extend for the new CHECK-COUNT-<num> directive.
• Convert DiagList and AnnotationList to std::vector, as probinson recommended.

In D52999#1301285, @probinson wrote:

It really seems like DiagList and AnnotationList ought to be vectors, not lists. They are append-only, and AnnotationList gets converted to a vector anyway to sort it. The code doesn't depend on the element-pointer stability guarantee of a list, except in one place noted below which can be fixed.
It's quite possible a vector would perform less well, in the face of many diags/annotations, but as the diags/annotations are the unusual case, performance is not really a big consideration.

In D53710#1293198, @probinson wrote:

I hadn't thought about -v; I don't think the new diag is currently under -v? The first test isn't using that option...

I think we should encourage people to have warnings on when they develop their tests. Having that separate from -v seems like a good thing, as -v obviously dumps a lot of info, most of which would ordinarily describe a test behaving exactly as intended.

Of course, let's see what Paul thinks before proceeding.

In D53710#1292880, @SjoerdMeijer wrote:

Agreed with all previous comments! And yes, I messed up my last lit test example :-( and I looked at yours and Paul's, and agree. But anyway, we have a plan...

In D53710#1291939, @probinson wrote:

we can't reliably report all typos.

In D53710#1291486, @SjoerdMeijer wrote:

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-)

Rebased onto updated patches earlier in series.

Rebased onto updated patches earlier in series.

Rebased onto updated patches earlier in series.

Rebased onto updated patches earlier in series.

Rebased onto updated patches earlier in series.

Rebased onto updated patches earlier in series.

• Rebased.
• Removed FILECHECK_DUMP_INPUT env var. The new FILECHECK_OPTS is sufficient.
• Removed unsetting of FILECHECK_* env vars in tests. That should be handled more carefully in a separate patch.

In D53710#1290508, @probinson wrote:

This seems like a reasonable use-case to me.

In D53710#1289937, @SjoerdMeijer wrote:

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Yep. Briefly looked at AMDGPU warnings. The very first 4 warnings, in tests basic-loop.ll, basic-call-return.ll, bug-vopc-commute.ll, call-return-types.ll, shows I think that they are dodgy tests, but then I see indeed the intentional use of the shared label prefix.

In D53517#1289308, @probinson wrote:

LGTM.
A patch to report an error on multiple -D of the same variable is probably a good thing, particularly since it doesn't work properly.

Address reviewer comments:

In D53710#1288598, @SjoerdMeijer wrote:

Friendly ping.

Does this look somewhat reasonable, and do what we want?

This is a small patch to FileCheck that I think could be beneficial throughout LLVM. Does it need an RFC?

• Address zturner's comment: prefer FILECHECK_DUMP_INPUT=never over -dump-input=never when avoiding user environment variables.
• Set FILECHECK_DUMP_INPUT=never in test/FileCheck/verbose.txt as well. Somehow I didn't notice this one was failing before.

• Split up the patch. This is now the first in the series. Each patch has a description and example of the diagnostics for which it implements input annotations.
• Add the input file name and check file name to the annotations description to further address a reviewer concern.
• Don't use the word "key" to identify the annotations description because technically that's not what it is.

In D52999#1271097, @jdenny wrote:

Ping.

@george.karpenkov: I don't know what your current plans are for this patch. I recall you once mentioned wanting it to set FileCheck's -v. D53517 could help with that, and it addresses other uses cases, such as color.

In D53710#1277482, @probinson wrote:

In D53710#1277353, @jdenny wrote:

Why isn't that just CHECK? Is that a legitimate use case?

CHECK-LABEL will complain if it doesn't find a matching string, so the test would fail if the label text does not appear.
However, it's not really a proper use of LABEL, and it would be better to have this test just use CHECK.

In D53710#1277094, @SjoerdMeijer wrote:

Thanks for looking at this! Answering this one first:

Have you run existing test suites to see how many warnings you get?

I just did, and it came back with some interesting results. For example test/CodeGen/ARM/Windows/mangling.ll:

; RUN: llc -mtriple=thumbv7-windows -mcpu=cortex-a9 -o - %s | FileCheck %s
define void @function() nounwind {
entry:
  ret void
}
; CHECK-LABEL: function

This shows that just checking the label is actually a use-case, which is why I thought a warning would be better; looks like we can't promote it to an error (which is okay I think).

Have you run existing test suites to see how many warnings you get? That would help to determine if there are use cases we haven't thought of. If there are no use cases, we might consider making this an error.

In D53419#1273329, @zturner wrote:

Seems ok to me, my only concern is that something could fail on Windows 8 or Windows 7, but I don't have a system to test that on.

The patch was reverted because the test it introduces fails on some Windows bots where color isn't supported as expected.

In D52999#1271022, @george.karpenkov wrote:

@jdenny

I'm willing to split up the patch more if it helps the review

Yes please, to be honest I can't still fully understand the feature description, let alone the code.
I have asked my colleague to take a glance, and his verdict was the same, so I'm not alone there.

You have a lot of new features in your FileCheck output; could you think of a way to separate them into simple, small, understandable chunks?

Adjusted to take advantage of D53419, which extends WithColor.

In D53419#1269236, @JDevlieghere wrote:

LGTM.

Address reviewer concerns:

In D52999#1260736, @george.karpenkov wrote:

OK that's somewhat more clear, but I'm still somewhat confused. Line by line:

$ FileCheck -v -dump-input=always checks1 < input1 |& sed -n '/^Key for/,$p'

I assume sed is there to suppress all output before the legend is printed?

In D52999#1259882, @george.karpenkov wrote:

@jdenny Sorry I'm still struggling to understand what exactly are you doing.