[libcxx-commits] [PATCH] D119152: [libc++] Implement P0627R6 (Function to mark unreachable code)

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added 1 blocking reviewer(s): ldionne.
Quuxplusone added a comment.

LGTM except for the missing `.verify.cpp` test. (And the accidental sed casualty `__libcpp_unreachable_sentinel`, which you're already aware of and fixing.)
Once the `.verify.cpp` test exists, I'd like @ldionne to approve too, since he had requested changes.



================
Comment at: libcxx/src/filesystem/filesystem_common.h:13
 #include "__config"
+#include <__utility/unreachable.h>
 #include "array"
----------------
philnik wrote:
> Quuxplusone wrote:
> > As you probably noticed, right now this file uses `#include "..."` over `#include <...>`, and doesn't include any detail headers directly. I think there's no particular reason to preserve these invariants, but probably we want to `s/""/<>/` in this file //before// we start mixing `<>` into it.
> Would you also be OK with me making a followup PR to clean up all the includes in `src/`?
This is now D119561


================
Comment at: libcxx/src/filesystem/operations.cpp:9
 
+#include <__utility/unreachable.h>
 #include "filesystem"
----------------
In `filesystem_common.h` you included the top-level `<utility>`; here and in `locale.cpp` you included only the detail header. I'm ambivalent whether we should consistently stick with top-level headers or consistently stick with the most granular detail headers; and in the process of making D119561 I discovered that we've already got a mix (although I think the granular ones are all due to @Mordante in `<format>`-related code). So, I'm ambivalent what to do here. No action required, I guess.


================
Comment at: libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include <utility>
----------------
philnik wrote:
> ldionne wrote:
> > This needs some standard requirements.
> > 
> > I suggest running just the test you're adding locally under various standard modes (`build/bin/llvm-lit -sv libcxx/test/<path-to-test> --param std=c++XX`) to shake out basic failures and get faster turn around time.
> Yeah I sometimes forget.
> However, I think we could do a verify.cpp test: namely,
> `[[noreturn]] void f() { std::unreachable(); }`
> should not trigger Clang's diagnostic `warning: function declared 'noreturn' should not return [-Winvalid-noreturn]`. If it does, that's a bug.

Please add a .verify.cpp test along these lines. I'm not sure exactly how it should look; please just make sure that if you //remove// `[[noreturn]]` from `__libcpp_unreachable`, and re-`ninja cxx`, and re-run the test, that the test correctly fails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119152