Address review comments

Author: LynxJinyangii

RcppTskit/NEWS.md

@@ -35,11 +35,6 @@ and releases adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html
   to append node rows from \code{R}, mirroring `tsk_node_table_add_row()`.
 - Added `rtsk_edge_table_add_row()` and `TableCollection$edge_table_add_row()`
   to append edge rows from \code{R}, mirroring `tsk_edge_table_add_row()`.
-- `TableCollection$node_table_add_row()` now maps `population = NULL` and
-  `individual = NULL` to `-1` (`TSK_NULL`) for R-level convenience.
-- `rtsk_edge_table_add_row()` and `TableCollection$edge_table_add_row()` now
-  validate `left`/`right` more explicitly (non-`NA`, finite, and `left < right`)
-  and require scalar `parent`/`child` IDs.
 - TODO
 
 ### Changed

RcppTskit/R/Class-TableCollection.R

@@ -213,7 +213,9 @@ TableCollection <- R6Class(
     #'   a raw vector, or a character of length 1.
     #' @details See the \code{tskit Python} equivalent at
     #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.NodeTable.add_row}.
-    #'   The function casts inputs to the expected class.
+    #'   The function casts inputs to the expected class. For convenience,
+    #'   \code{population = NULL} and \code{individual = NULL} are mapped to
+    #'   \code{-1} (\code{TSK_NULL}).
     #' @return Integer row ID (0-based) of the newly added node.
     #' @examples
     #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")
@@ -275,7 +277,10 @@ TableCollection <- R6Class(
     #'   a raw vector, or a character of length 1.
     #' @details See the \code{tskit Python} equivalent at
     #'   \url{https://tskit.dev/tskit/docs/stable/python-api.html#tskit.EdgeTable.add_row}.
-    #'   The function casts inputs to the expected class.
+    #'   The function casts inputs to the expected class. Inputs are validated:
+    #'   \code{left} and \code{right} must be finite numeric scalars with
+    #'   \code{left < right}, and \code{parent} and \code{child} must be
+    #'   non-\code{NA} integer scalars.
     #' @return Integer row ID (0-based) of the newly added edge.
     #' @examples
     #' ts_file <- system.file("examples/test.trees", package = "RcppTskit")

RcppTskit/R/RcppExports.R

@@ -251,3 +251,11 @@ test_rtsk_individual_table_add_row_forced_error <- function(tc) {
     invisible(.Call(`_RcppTskit_test_rtsk_individual_table_add_row_forced_error`, tc))
 }
 
+test_rtsk_node_table_add_row_forced_error <- function(tc) {
+    invisible(.Call(`_RcppTskit_test_rtsk_node_table_add_row_forced_error`, tc))
+}
+
+test_rtsk_edge_table_add_row_forced_error <- function(tc) {
+    invisible(.Call(`_RcppTskit_test_rtsk_edge_table_add_row_forced_error`, tc))
+}
+

RcppTskit/man/TableCollection.Rd

@@ -669,6 +669,26 @@ BEGIN_RCPP
     return R_NilValue;
 END_RCPP
 }
+// test_rtsk_node_table_add_row_forced_error
+void test_rtsk_node_table_add_row_forced_error(const SEXP tc);
+RcppExport SEXP _RcppTskit_test_rtsk_node_table_add_row_forced_error(SEXP tcSEXP) {
+BEGIN_RCPP
+    Rcpp::RNGScope rcpp_rngScope_gen;
+    Rcpp::traits::input_parameter< const SEXP >::type tc(tcSEXP);
+    test_rtsk_node_table_add_row_forced_error(tc);
+    return R_NilValue;
+END_RCPP
+}
+// test_rtsk_edge_table_add_row_forced_error
+void test_rtsk_edge_table_add_row_forced_error(const SEXP tc);
+RcppExport SEXP _RcppTskit_test_rtsk_edge_table_add_row_forced_error(SEXP tcSEXP) {
+BEGIN_RCPP
+    Rcpp::RNGScope rcpp_rngScope_gen;
+    Rcpp::traits::input_parameter< const SEXP >::type tc(tcSEXP);
+    test_rtsk_edge_table_add_row_forced_error(tc);
+    return R_NilValue;
+END_RCPP
+}
 
 static const R_CallMethodDef CallEntries[] = {
     {"_RcppTskit_test_validate_options", (DL_FUNC) &_RcppTskit_test_validate_options, 2},
@@ -730,6 +750,8 @@ static const R_CallMethodDef CallEntries[] = {
     {"_RcppTskit_test_rtsk_treeseq_init_forced_error", (DL_FUNC) &_RcppTskit_test_rtsk_treeseq_init_forced_error, 1},
     {"_RcppTskit_test_rtsk_table_collection_build_index_forced_error", (DL_FUNC) &_RcppTskit_test_rtsk_table_collection_build_index_forced_error, 1},
     {"_RcppTskit_test_rtsk_individual_table_add_row_forced_error", (DL_FUNC) &_RcppTskit_test_rtsk_individual_table_add_row_forced_error, 1},
+    {"_RcppTskit_test_rtsk_node_table_add_row_forced_error", (DL_FUNC) &_RcppTskit_test_rtsk_node_table_add_row_forced_error, 1},
+    {"_RcppTskit_test_rtsk_edge_table_add_row_forced_error", (DL_FUNC) &_RcppTskit_test_rtsk_edge_table_add_row_forced_error, 1},
     {NULL, NULL, 0}
 };
 

RcppTskit/src/RcppExports.cpp

@@ -149,3 +149,60 @@ void test_rtsk_individual_table_add_row_forced_error(const SEXP tc) {
     throw;
   }
 }
+
+// TEST-ONLY
+// @title Force tskit-level error path in \code{rtsk_node_table_add_row}
+// @param tc an external pointer to table collection as a
+//   \code{tsk_table_collection_t} object.
+// @return No return value; called for side effects - testing.
+// [[Rcpp::export]]
+void test_rtsk_node_table_add_row_forced_error(const SEXP tc) {
+  rtsk_table_collection_t tc_xptr(tc);
+  tsk_node_table_t &nodes = tc_xptr->nodes;
+  tsk_size_t saved_max_rows = nodes.max_rows;
+  tsk_size_t saved_max_rows_increment = nodes.max_rows_increment;
+  nodes.max_rows = 1;
+  nodes.max_rows_increment = static_cast<tsk_size_t>(TSK_MAX_ID) + 1;
+  try {
+    (void)rtsk_node_table_add_row(tc);
+    // Lines below not hit by tests because rtsk_node_table_add_row()
+    // throws error # nocov start
+    nodes.max_rows = saved_max_rows;
+    nodes.max_rows_increment = saved_max_rows_increment;
+    return;
+    // # nocov end
+  } catch (...) {
+    nodes.max_rows = saved_max_rows;
+    nodes.max_rows_increment = saved_max_rows_increment;
+    throw;
+  }
+}
+
+// TEST-ONLY
+// @title Force tskit-level error path in \code{rtsk_edge_table_add_row}
+// @param tc an external pointer to table collection as a
+//   \code{tsk_table_collection_t} object.
+// @return No return value; called for side effects - testing.
+// [[Rcpp::export]]
+void test_rtsk_edge_table_add_row_forced_error(const SEXP tc) {
+  rtsk_table_collection_t tc_xptr(tc);
+  tsk_edge_table_t &edges = tc_xptr->edges;
+  tsk_size_t saved_max_rows = edges.max_rows;
+  tsk_size_t saved_max_rows_increment = edges.max_rows_increment;
+  edges.max_rows = 1;
+  edges.max_rows_increment = static_cast<tsk_size_t>(TSK_MAX_ID) + 1;
+  try {
+    (void)rtsk_edge_table_add_row(tc, 0, 1, static_cast<int>(edges.parent[0]),
+                                  static_cast<int>(edges.child[0]), R_NilValue);
+    // Lines below not hit by tests because rtsk_edge_table_add_row()
+    // throws error # nocov start
+    edges.max_rows = saved_max_rows;
+    edges.max_rows_increment = saved_max_rows_increment;
+    return;
+    // # nocov end
+  } catch (...) {
+    edges.max_rows = saved_max_rows;
+    edges.max_rows_increment = saved_max_rows_increment;
+    throw;
+  }
+}

RcppTskit/src/tests.cpp

@@ -549,6 +549,10 @@ test_that("node_table_add_row wrapper expands the table collection and handles i
     tc$node_table_add_row(metadata = 1L),
     regexp = "metadata must be NULL, a raw vector, or a length-1 non-NA character string!"
   )
+  expect_error(
+    test_rtsk_node_table_add_row_forced_error(tc$xptr),
+    regexp = "TSK_ERR_TABLE_OVERFLOW"
+  )
 })
 
 test_that("edge_table_add_row wrapper expands the table collection and handles inputs", {
@@ -649,6 +653,16 @@ test_that("edge_table_add_row wrapper expands the table collection and handles i
     ),
     regexp = "left must be finite in rtsk_edge_table_add_row"
   )
+  expect_error(
+    rtsk_edge_table_add_row(
+      tc = tc_xptr,
+      left = 0,
+      right = Inf,
+      parent = parent,
+      child = child
+    ),
+    regexp = "right must be finite in rtsk_edge_table_add_row"
+  )
   expect_error(
     rtsk_edge_table_add_row(
       tc = tc_xptr,
@@ -827,4 +841,8 @@ test_that("edge_table_add_row wrapper expands the table collection and handles i
     ),
     regexp = "metadata must be NULL, a raw vector, or a length-1 non-NA character string!"
   )
+  expect_error(
+    test_rtsk_edge_table_add_row_forced_error(tc$xptr),
+    regexp = "TSK_ERR_TABLE_OVERFLOW"
+  )
 })