This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Remove disable_missing_braces_warning.h from tests
ClosedPublic

Authored by jloser on Sep 13 2021, 11:30 AM.

Details

Summary

Several tests include disable_missing_braces_warning.h but do not need
to. Remove the include.

Inspired from discussion at https://reviews.llvm.org/D109668

Diff Detail

Event Timeline

jloser requested review of this revision.Sep 13 2021, 11:30 AM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2021, 11:30 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

If buildkite is actually OK with this, then I'm OK with it.

But I assume that buildkite would not be OK with this as-is; I assume it'll produce that warning diagnostic. But I think we should fix the warning appropriately, e.g. by changing const std::array<long, 5> arr = {4, 5, 6, 7, 8}; to const std::array<long, 5> arr = {{4, 5, 6, 7, 8}};. I believe such changes will be trivially easy, except for apply_extended_types.pass.cpp, where the missing braces are inside a template that's used sometimes with array (which needs double braces) and sometimes with tuple (which needs single braces).

I don't understand why you changed only these few tests. E.g. why not any of the tests under std/containers/sequences/array/? and why not std/iterators/iterator.range/begin-end.pass.cpp?

Mordante accepted this revision as: Mordante.Sep 15 2021, 10:31 AM
Mordante added a subscriber: Mordante.

LGTM. But I too would prefer remove all no longer needed #include "disable_missing_braces_warning.h" in this commit and remove the then obsolete header.

@Quuxplusone Am I missing something? AFAIK const std::array<long, 5> arr = {4, 5, 6, 7, 8}; is valid C++11.

@Quuxplusone Am I missing something? AFAIK const std::array<long, 5> arr = {4, 5, 6, 7, 8}; is valid C++11.

It's definitely valid, but I had assumed (wrongly, it seems) that it would still trigger a non-fatal warning on Clang and/or GCC, which would have been the point of the header: to avoid that non-fatal warning.
However, at least on Godbolt,

  • I cannot reproduce any such warning on Clang at all
  • I can reproduce it on GCC but only by explicitly passing -Wmissing-braces; it's not part of -Wall -Wextra

So yeah, @jloser, can we see what happens if you just git rm disable_missing_braces_warning.h and remove all its uses? I now predict that we'll see buildkite totally happy with that.

ldionne requested changes to this revision.Sep 20 2021, 10:27 AM

Agreed with the other reviewers, please try removing disable_missing_braces_warning,h entirely.

This revision now requires changes to proceed.Sep 20 2021, 10:27 AM
jloser updated this revision to Diff 373686.Sep 20 2021, 12:12 PM
jloser retitled this revision from [libc++][test] Remove disable_missing_braces_warning.h from few tests to [libc++][test] Remove disable_missing_braces_warning.h from tests.
jloser edited the summary of this revision. (Show Details)

Remove rest of disable_missing_braces_warning.h from other tests.
Remove header disable_missing_braces_warning.h

ldionne accepted this revision.Sep 20 2021, 1:54 PM

Thanks! LGTM if CI passes.

This revision is now accepted and ready to land.Sep 20 2021, 1:54 PM

Thanks! LGTM if CI passes.

CI is failing in the runtimes build (https://buildkite.com/llvm-project/libcxx-ci/builds/5444#eabe837f-7b55-4767-a14b-24ef963ad3dc) but this looks unrelated to my changes. I didn't touch this test so its behavior should be unchanged. Safe to land this, or what would you like me to do?

jloser updated this revision to Diff 373903.Sep 21 2021, 6:53 AM

Rebase to kick the bots

jloser updated this revision to Diff 374208.Sep 22 2021, 6:41 AM

Rebase with main to fix CI

jloser updated this revision to Diff 374250.Sep 22 2021, 8:50 AM

Poke buildbots due to failure from out-of-space on build bot

jloser updated this revision to Diff 374333.Sep 22 2021, 12:47 PM

Kick the bots again...

This revision was landed with ongoing or failed builds.Sep 22 2021, 1:00 PM
This revision was automatically updated to reflect the committed changes.