From 8971bda0c2b124ab837818d7a57ce3c87f8d5983 Mon Sep 17 00:00:00 2001 From: nick black Date: Fri, 22 May 2020 03:31:03 -0400 Subject: [PATCH] ncpp: stop() resets _instance, add unit test #538 --- CMakeLists.txt | 2 +- include/ncpp/NotCurses.hh | 15 +--------- src/lib/fd.c | 6 ++-- src/libcpp/NotCurses.cc | 18 ++++++++++++ src/poc/ncpp_build.cpp | 10 ++++--- src/poc/ncpp_build_exceptions.cpp | 10 ++++--- src/tetris/main.h | 36 ++++++++++++++---------- tests/{exceptions.cpp => Exceptions.cpp} | 9 ++++++ tests/Ncpp.cpp | 28 ++++++++++++++++++ tests/fade.cpp | 1 + tests/fds.cpp | 4 ++- tests/main.cpp | 4 +++ tests/main.h | 1 + tests/ncpp.cpp | 16 ----------- 14 files changed, 103 insertions(+), 57 deletions(-) rename tests/{exceptions.cpp => Exceptions.cpp} (54%) create mode 100644 tests/Ncpp.cpp delete mode 100644 tests/ncpp.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 329f62116..7fc1c5c87 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -56,7 +56,7 @@ endif() # global compiler flags add_compile_definitions(FORTIFY_SOURCE=2) -add_compile_options(-Wall -Wextra -W -Wshadow -Wformat) +add_compile_options(-Wall -Wextra -W -Wshadow -Wformat -fexceptions) message(STATUS "Requested multimedia engine: ${USE_MULTIMEDIA}") message(STATUS "Requested build mode: ${CMAKE_BUILD_TYPE}") diff --git a/include/ncpp/NotCurses.hh b/include/ncpp/NotCurses.hh index 99e3566a2..d256cd137 100644 --- a/include/ncpp/NotCurses.hh +++ b/include/ncpp/NotCurses.hh @@ -81,20 +81,7 @@ namespace ncpp return notcurses_version (); } - // This is potentially dangerous, but alas necessary. It can cause other calls here to fail in a bad way, but we - // need a way to report errors to std{out,err} in case of failure and that will work only if notcurses is - // stopped, so... - // - bool stop () - { - if (nc == nullptr) - throw invalid_state_error (ncpp_invalid_state_message); - - bool ret = !notcurses_stop (nc); - nc = nullptr; - - return ret; - } + bool stop (); bool can_fade () const noexcept { diff --git a/src/lib/fd.c b/src/lib/fd.c index 8a0bd99fb..00eb4acd7 100644 --- a/src/lib/fd.c +++ b/src/lib/fd.c @@ -145,6 +145,7 @@ int ncfdplane_destroy(ncfdplane* n){ static pid_t launch_pipe_process(int* pipe, int* pidfd){ + *pidfd = -1; int pipes[2]; if(pipe2(pipes, O_CLOEXEC)){ // can't use O_NBLOCK here (affects client) return -1; @@ -166,7 +167,6 @@ launch_pipe_process(int* pipe, int* pidfd){ if(p == 0){ // child if(dup2(pipes[1], STDOUT_FILENO) < 0 || dup2(pipes[1], STDERR_FILENO) < 0){ fprintf(stderr, "Couldn't dup() %d (%s)\n", pipes[1], strerror(errno)); - raise(SIGKILL); exit(EXIT_FAILURE); } }else if(p > 0){ // parent @@ -245,6 +245,7 @@ ncsubproc* ncsubproc_createv(ncplane* n, const ncsubproc_options* opts, if(ret == NULL){ return NULL; } + memset(ret, 0, sizeof(*ret)); ret->pid = launch_pipe_process(&fd, &ret->pidfd); if(ret->pid == 0){ execv(bin, arg); @@ -273,11 +274,11 @@ ncsubproc* ncsubproc_createvp(ncplane* n, const ncsubproc_options* opts, if(ret == NULL){ return NULL; } + memset(ret, 0, sizeof(*ret)); ret->pid = launch_pipe_process(&fd, &ret->pidfd); if(ret->pid == 0){ execvp(bin, arg); fprintf(stderr, "Error execv()ing %s\n", bin); - raise(SIGKILL); exit(EXIT_FAILURE); }else if(ret->pid < 0){ free(ret); @@ -302,6 +303,7 @@ ncsubproc* ncsubproc_createvpe(ncplane* n, const ncsubproc_options* opts, if(ret == NULL){ return NULL; } + memset(ret, 0, sizeof(*ret)); ret->pid = launch_pipe_process(&fd, &ret->pidfd); if(ret->pid == 0){ #ifdef __FreeBSD__ diff --git a/src/libcpp/NotCurses.cc b/src/libcpp/NotCurses.cc index 0877cc1a5..cab4fa52c 100644 --- a/src/libcpp/NotCurses.cc +++ b/src/libcpp/NotCurses.cc @@ -54,3 +54,21 @@ Plane* NotCurses::get_top () noexcept return Plane::map_plane (top); } + +// This is potentially dangerous, but alas necessary. It can cause other calls +// here to fail in a bad way, but we need a way to report errors to +// std{out,err} in case of failure and that will work only if notcurses is +// stopped, so... +bool NotCurses::stop () +{ + if (nc == nullptr) + throw invalid_state_error (ncpp_invalid_state_message); + + bool ret = !notcurses_stop (nc); + nc = nullptr; + + const std::lock_guard lock (init_mutex); + _instance = nullptr; + + return ret; +} diff --git a/src/poc/ncpp_build.cpp b/src/poc/ncpp_build.cpp index 0e0d10a79..698013209 100644 --- a/src/poc/ncpp_build.cpp +++ b/src/poc/ncpp_build.cpp @@ -26,10 +26,12 @@ int run () NotCurses nc; const char *ncver = nc.version (); - Plane plane (1, 1, 0, 0); - Plot plot1 (plane); - PlotU plot2 (plane); - PlotD plot3 (plane); + { + Plane plane (1, 1, 0, 0); + Plot plot1 (plane); + PlotU plot2 (plane); + PlotD plot3 (plane); + } nc.stop (); diff --git a/src/poc/ncpp_build_exceptions.cpp b/src/poc/ncpp_build_exceptions.cpp index 25ae70e76..f08d93e98 100644 --- a/src/poc/ncpp_build_exceptions.cpp +++ b/src/poc/ncpp_build_exceptions.cpp @@ -27,10 +27,12 @@ int run () NotCurses nc; const char *ncver = nc.version (); - Plane plane (1, 1, 0, 0); - Plot plot1 (plane); - PlotU plot2 (plane); - PlotD plot3 (plane); + { + Plane plane (1, 1, 0, 0); + Plot plot1 (plane); + PlotU plot2 (plane); + PlotD plot3 (plane); + } nc.stop (); diff --git a/src/tetris/main.h b/src/tetris/main.h index 0b7a211ed..ceb9ef963 100644 --- a/src/tetris/main.h +++ b/src/tetris/main.h @@ -1,14 +1,4 @@ -int main(void) { - if(setlocale(LC_ALL, "") == nullptr){ - return EXIT_FAILURE; - } - srand(time(nullptr)); - std::atomic_bool gameover = false; - notcurses_options ncopts{}; - ncopts.flags = NCOPTION_INHIBIT_SETLOCALE; - ncpp::NotCurses nc(ncopts); - Tetris t{nc, gameover}; - std::thread tid(&Tetris::Ticker, &t); +bool IOLoop(ncpp::NotCurses& nc, Tetris& t, std::atomic_bool& gameover) { ncpp::Plane* stdplane = nc.get_stdplane(); char32_t input = 0; ncinput ni; @@ -32,11 +22,27 @@ int main(void) { } ncmtx.unlock(); } - if(gameover || input == 'q'){ // FIXME signal it on 'q' - gameover = true; - tid.join(); - }else{ + return gameover || input == 'q'; +} + +int main(void) { + if(setlocale(LC_ALL, "") == nullptr){ return EXIT_FAILURE; } + srand(time(nullptr)); + std::atomic_bool gameover = false; + notcurses_options ncopts{}; + ncopts.flags = NCOPTION_INHIBIT_SETLOCALE; + ncpp::NotCurses nc(ncopts); + { + Tetris t{nc, gameover}; + std::thread tid(&Tetris::Ticker, &t); + if(IOLoop(nc, t, gameover)){ + gameover = true; // FIXME signal thread + tid.join(); + }else{ + return EXIT_FAILURE; + } + } return nc.stop() ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/exceptions.cpp b/tests/Exceptions.cpp similarity index 54% rename from tests/exceptions.cpp rename to tests/Exceptions.cpp index da930361f..a4f11a10c 100644 --- a/tests/exceptions.cpp +++ b/tests/Exceptions.cpp @@ -15,4 +15,13 @@ TEST_CASE("Exceptions") { CHECK(nc.stop()); } + // ncpp only allows one notcurses object at a time (why?) + SUBCASE("OnlyOneNotCurses") { + NotCurses nc; + std::unique_ptr nc2; + // FIXME attempts to match ::init_error have failed thus far :/ + CHECK_THROWS(nc2.reset(new NotCurses())); + CHECK(nc.stop()); + } + } diff --git a/tests/Ncpp.cpp b/tests/Ncpp.cpp new file mode 100644 index 000000000..4453d16ec --- /dev/null +++ b/tests/Ncpp.cpp @@ -0,0 +1,28 @@ +#include "main.h" + +using namespace ncpp; + +TEST_CASE("Ncpp" + * doctest::description("Basic C++ wrapper tests")) { + + // we ought be able to construct a NotCurses object with a nullptr FILE + // or even just no argument (decays to nullptr). + SUBCASE("ConstructNotCurses") { + NotCurses nc; + CHECK(nc.stop()); + } + + SUBCASE("ConstructNotCursesNullFILE") { + NotCurses ncnull(nullptr); + CHECK(ncnull.stop()); + } + + // we ought be able to get a new NotCurses object after stop()ping one. + SUBCASE("ConstructNotCursesTwice") { + NotCurses nc; + CHECK(nc.stop()); + NotCurses ncnull(nullptr); + CHECK(ncnull.stop()); + } + +} diff --git a/tests/fade.cpp b/tests/fade.cpp index 84e06cd08..9f4abf42b 100644 --- a/tests/fade.cpp +++ b/tests/fade.cpp @@ -28,6 +28,7 @@ TEST_CASE("Fade") { struct ncplane* n_ = notcurses_stdplane(nc_); REQUIRE(n_); if(!notcurses_canfade(nc_)){ + CHECK(0 == notcurses_stop(nc_)); return; } REQUIRE(0 == ncplane_cursor_move_yx(n_, 0, 0)); diff --git a/tests/fds.cpp b/tests/fds.cpp index 93f61afa4..9444e4dd6 100644 --- a/tests/fds.cpp +++ b/tests/fds.cpp @@ -47,7 +47,9 @@ auto testfdeofdestroys(struct ncfdplane* n, int fderrno, void* curry) -> int { } // test ncfdplanes and ncsubprocs -TEST_CASE("FdsAndSubprocs") { +TEST_CASE("FdsAndSubprocs" + * doctest::description("Fdplanes and subprocedures") + * doctest::timeout(10)) { notcurses_options nopts{}; nopts.inhibit_alternate_screen = true; nopts.suppress_banner = true; diff --git a/tests/main.cpp b/tests/main.cpp index 193aea3a6..c6e5090c3 100644 --- a/tests/main.cpp +++ b/tests/main.cpp @@ -90,6 +90,10 @@ auto main(int argc, const char **argv) -> int { std::cerr << "Coudln't set locale based on user preferences!" << std::endl; return EXIT_FAILURE; } + if(getenv("TERM") == NULL){ + std::cerr << "TERM wasn't defined, exiting with success" << std::endl; + return EXIT_SUCCESS; + } doctest::Context context; context.setOption("order-by", "name"); // sort the test cases by their name diff --git a/tests/main.h b/tests/main.h index 500ed8e9a..7f2662e9e 100644 --- a/tests/main.h +++ b/tests/main.h @@ -6,6 +6,7 @@ #include #include #include +#include #include "internal.h" auto find_data(const char* datum) -> char*; diff --git a/tests/ncpp.cpp b/tests/ncpp.cpp deleted file mode 100644 index 7600b229d..000000000 --- a/tests/ncpp.cpp +++ /dev/null @@ -1,16 +0,0 @@ -#include "main.h" - -using namespace ncpp; - -TEST_CASE("Ncpp") { - - // we ought be able to construct a NotCurses object with a nullptr FILE - // or even just no argument (decays to nullptr). - SUBCASE("ConstructNullFILE") { - NotCurses nc; - CHECK(nc.stop()); - NotCurses ncnull(nullptr); - CHECK(ncnull.stop()); - } - -}