ncpp: stop() resets _instance, add unit test #538

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}")

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
 		{

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__

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<std::mutex> lock (init_mutex);
+  _instance = nullptr;
+
+  return ret;
+}

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 ();
 

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 ();
 

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;
 }

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<NotCurses> nc2;
+    // FIXME attempts to match ::init_error have failed thus far :/
+    CHECK_THROWS(nc2.reset(new NotCurses()));
+    CHECK(nc.stop());
+  }
+
 }

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());
+  }
+
+}

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));

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;

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

tests/main.h

@@ -6,6 +6,7 @@
 #include <doctest/doctest.h>
 #include <notcurses/notcurses.h>
 #include <ncpp/NotCurses.hh>
+#include <ncpp/_exceptions.hh>
 #include "internal.h"
 
 auto find_data(const char* datum) -> char*;

tests/ncpp.cpp

@@ -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());
-  }
-
-}