[libcxx-commits] [PATCH] D99515: [libc++] Build and test with -Wundef warning. NFC.

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

Quuxplusone added a comment.

LGTM mod nits.
It's unfortunate that we can't build the entire library with `-Wundef` — or rather, if we did, we'd have to go through and replace a //ton// of `X > Y` with `defined(X) && X > Y`, and I don't think it'd end up being worth it. This PR shows that `-Wundef` //does// catch real bugs, though!



================
Comment at: libcxx/CMakeLists.txt:580
   else()
-    target_add_compile_flags_if_supported(${target} PRIVATE -Wall)
+    target_add_compile_flags_if_supported(${target} PRIVATE -Wall -Wundef)
   endif()
----------------
curdeius wrote:
> Maybe that's not the best place to put this flag... Should I put it twice for clang (line 586+), and gcc (line 611+)?
It looks to me as if you should put it in the big list of "if supported" flags on lines 582–584, alongside `-Wwrite-strings` and `-Wextra-semi`.


================
Comment at: libcxx/include/future:505
+#if _LIBCPP_STD_VER > 14
+    // explicit future_error(future_errc _Ev) : logic_error(), __ec_(make_error_code(_Ev)) {}
 #endif
----------------
curdeius wrote:
> This was already dead code, so just commented it out in this patch.
> I'll create another patch to fix this if that's wanted. However, the `future_error(error_code __ec);` gets called with arguments of type `future_errc`, so adding `future_error(future_errc)` ctor as `future_error(future_errc _Ev) : future_error(make_error_code(_Ev) {}` will not add anything.
> We might however want to do something more standard conformant (mind that `future(error_code)` ctor is "exposition only").
I recommend just deleting lines 504–506. That clearly preserves the existing behavior (which is not buggy AFAWCT, right?). If the existing behavior //is// buggy, then you should add a test that detects the bug.
I would not check in commented-out code.


================
Comment at: libcxx/test/std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq_all_zero.pass.cpp:33
+#include <initializer_list>
 #endif
 
----------------
I recommend just omitting the `TEST_STD_VER` check entirely, and/or omitting the entire lines 28–30. As D99309 shows, <algorithm> is already guaranteed to include <initializer_list>; we don't have to re-include it if re-including it complicates the code (which it seems it does).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515