[PATCH] D23156: [ADT] Stop trying to test every combination of values in a triple in every permutation.

[Dean Michael Berris via llvm-commits]

dberris added a comment.

In https://reviews.llvm.org/D23156#506420, @chandlerc wrote:

> In https://reviews.llvm.org/D23156#506253, @dblaikie wrote:
>
> >
>
>
>
>
> > - would it be reasonable to assume that the parsing of a value is independent of which slot it is in? So we shouldn't then test that every value can appear in every slot/ordering?
>
>
> It might, but that just saves 4! from this and wouldn't catch stateful bugs in the parsing. So I'm pretty happy to try and parse each entry in each position.
>
> Mostly this is because the runtime is now so fast that it seems to not matter.
>
> Does that make sense?


I think that makes sense, although catching state-related bugs might be possible through a different test -- i.e. specifically testing boundary conditions for when a buggy implementation might create a wrongly-normalised triple.

Although I do agree that this is better than the current state.

Mind updating the description with some of the computations, to make it clearer what kinds of savings (in terms of test cases) and what kinds of trade-offs are being made? More specifically I suggest re-wording the parts about combinations and permutations, and using the formulae instead (even if they're hand-wavy and approximations).


================
Comment at: unittests/ADT/TripleTest.cpp:306-310
@@ +305,7 @@
+  //
+  // We don't check every possible combination because that set is ever growing
+  // and intractable to test quickly. But we check every option for any given
+  // slot and make sure it gets normalized to the correct position from every
+  // permutation. This should cover the core logic while being a tractable
+  // number of tests.
+  auto FirstArchType = Triple::ArchType(Triple::UnknownArch + 1);
----------------
I think in-lining the computation in the reduction of the number of tests here would be useful (instead of just having it in the description). For example, it's useful to know that if we do the Cartesian Product and permute each of the tuples in that space, we get the following number of test-cases:

    X = (|Arch| * |Vendor| * |OS| * |Env|) * 4!

This patch cuts it down to:

    Y = (|Arch| + |Vendor| + |OS| + |Env|) * 4! * 4

That's the difference between a geometric series and an arithmetic series -- i.e. if any one of the sets grows by N, that X grows exponentially compared to linearly to Y.


https://reviews.llvm.org/D23156