This is an archive of the discontinued LLVM Phabricator instance.

Add testing for Fuchsia multilib
AbandonedPublic

Authored by michaelplatings on Jan 30 2023, 1:59 AM.

Details

Reviewers
phosek
Summary

The plan is to change how multilib works under the hood and this test
gives confidence that the changes won't break which multilib variant is
selected.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:59 AM
Herald added a subscriber: abrachet. · View Herald Transcript
michaelplatings requested review of this revision.Jan 30 2023, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:59 AM

Update commit message and run arc with clang-format on the PATH

michaelplatings edited the summary of this revision. (Show Details)Jan 30 2023, 2:07 AM
phosek added a comment.Feb 1 2023, 9:23 AM

Thanks, some of these are no longer needed which could simplify this change so I sent out D143050 to clean up the Fuchsia driver first.

phosek added inline comments.Feb 4 2023, 11:03 PM
clang/lib/Driver/ToolChains/Fuchsia.cpp
308–309

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.

clang/unittests/Driver/FuchsiaTest.cpp
19–24

Nit: we prefer // for comments.

31–44

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.

michaelplatings abandoned this revision.Feb 6 2023, 3:30 AM

A more scalable approach would be to check only the minimal set of combination necessary to achieve full coverage.

In that case the testing you've already got in place in clang/test/Driver/fuchsia.cpp is adequate. There was a potential bug here so I was being extra cautious, but in practice the test added in this change didn't pick up anything that wasn't also caught by the existing tests. I'll abandon this change and remove it from the stack. The change stack now begins at D142893.