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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__utility/unreachable.h:18
+{
+#ifdef __GNUC__
+  __builtin_unreachable();
----------------
ldionne wrote:
> I believe we should use `__has_builtin(__builtin_unreachable)` here instead -- that would avoid hardcoding compiler knowledge.
IMHO we should name this function `__libcpp_unreachable`; just `__unreachable` seems in danger of stepping on some compiler's builtin or something.
Otherwise, I have no objection to this approach. Looking in `<cstdlib>`, it turns out that it was already impossible for users to `-D_LIBCPP_UNREACHABLE=anything-else`, so we're not even breaking any unsupported-but-working thing any user might be doing. And the call sites don't need `std::` because it takes no arguments, so ADL isn't a problem.


================
Comment at: libcxx/include/charconv:90
 #include <cstdint>
 #include <cstdlib> // for _LIBCPP_UNREACHABLE
 #include <cstring>
----------------
Can we remove this? If not, let's at least remove the comment (which is wrong now).


================
Comment at: libcxx/include/fstream:188
 #include <cstdio>
 #include <cstdlib>
 #include <istream>
----------------
Can we remove this now? (The answer, if "yes", will depend partly on politics: are we OK with breaking users who `#include <fstream>` and then call `malloc` or whatever.)


================
Comment at: libcxx/src/filesystem/filesystem_common.h:13
 #include "__config"
+#include <__utility/unreachable.h>
 #include "array"
----------------
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.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:18
 #include "cstdarg"
 #include "cstdlib"
 #include "ctime"
----------------
Can we remove this now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119152