[PATCH] D142878: Add testing for Fuchsia multilib

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Fuchsia.cpp:316-317
+void Fuchsia::configureMultilibFlags(Multilib::flags_list &Flags,
+                                     bool Exceptions, bool Asan, bool Hwasan,
+                                     bool Itanium) {
+  addMultilibFlag(Exceptions, "fexceptions", Flags);
----------------
Could we pass `const ArgList &Args` instead? I'm concerned that this approach won't scale in the future as we keep on adding more flags.


================
Comment at: clang/unittests/Driver/FuchsiaTest.cpp:18-23
+/*
+This test was added prior to changing the behaviour of Multilib.
+The way that Fuchsia used Multilib made it very likely that the change
+would cause it to break so by adding this exhaustive test we avoid that
+possibility.
+*/
----------------
Nit: we prefer `//` for comments.


================
Comment at: clang/unittests/Driver/FuchsiaTest.cpp:30-43
+  for (bool Itanium : {false, true}) {
+    for (bool Hwasan : {false, true}) {
+      for (bool Asan : {false, true}) {
+        for (bool Exceptions : {false, true}) {
+          Multilib::flags_list Flags;
+          toolchains::Fuchsia::configureMultilibFlags(Flags, Exceptions, Asan,
+                                                      Hwasan, Itanium);
----------------
While exhaustive, I'm concerned about the scalability of this approach. We plan on adding more multilibs in the future so the number of combinations is going to grow exponentially. A more scalable approach would be to check only the minimal set of combination necessary to achieve full coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142878