[PATCH] D90281: [FileCheck] Report missing prefixes when more than one is provided.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D90281#2358359 <https://reviews.llvm.org/D90281#2358359>, @grimar wrote:

> In D90281#2358188 <https://reviews.llvm.org/D90281#2358188>, @jdoerfert wrote:
>
>> In D90281#2358184 <https://reviews.llvm.org/D90281#2358184>, @mtrofin wrote:
>>
>>> [...]
>>> If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?
>>
>> The run lines are written/copied by us. We could easily switch to a new option like that.
>
> I think we could add an option like `--allow-unused-prefixes`. It is probably sounds a bit simpler and can easily be added to generator I think.

That's a better name, indeed.

The remaining question is how to stage this. I think there are 2 broad ways to do it:

1. we enable it by default, and this patch goes and patches all failing tests with an opt-out.
2. we start with it disabled. Then, people opt in explicitly.

The problem I see with "2" is that there's no pressure to move to the "strict" place, which I think is what we want for a test suite.

The problem with "1" is 1500 failures. I think the way to mitigate that would be:

- we start with the flag, and it does nothing
- we annotate the tests (I can provide the list) - and this can be parallelized (i.e. perhaps folks can help with this)
- we then land this patch

My preference is to go with nr "1".
What do folks think?



================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1830
+  StringSet<> PrefixesNotFound;
+  for (auto CP : Req.CheckPrefixes)
+    PrefixesNotFound.insert(CP);
----------------
jhenderson wrote:
> I'd prefer no `auto` here - what's the type of `CP`?
removed this.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1942
+    bool First = true;
+    for (auto P : PrefixesNotFound.keys()) {
+      if (!First)
----------------
grimar wrote:
> `StringMap` iteration order is not guaranteed to be deterministic. I think
> some different container like `MapVector` probably should be used here instead.
How about a std::set - it addresses determinism, the code is more readable (no "PrefixesNotFound.keys()" abstraction leaks), and any overhead it may have is likely not an issue, as we don't expect the prefixes list to be too large.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90281