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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
