Merge pull request #72 from r-spatial/dewey-dev

Fix CRAN check errors, update actions

Author: paleolimbot

.github/workflows/R-CMD-check.yaml

@@ -21,16 +21,15 @@ jobs:
         config:
           - {os: windows-latest, r: '3.6'}
           - {os: macOS-latest, r: '3.6'}
-          - {os: ubuntu-16.04, r: '3.6'}
-          - {os: ubuntu-18.04, r: '3.6'}
           - {os: windows-latest, r: '4.0'}
           - {os: macOS-latest, r: '4.0'}
-          - {os: ubuntu-16.04, r: '4.0'}
-          - {os: ubuntu-18.04, r: '4.0'}
+          - {os: ubuntu-16.04, r: '4.0', rspm: "https://packagemanager.rstudio.com/cran/__linux__/xenial/latest"}
+          - {os: ubuntu-18.04, r: '4.0', rspm: "https://packagemanager.rstudio.com/cran/__linux__/bionic/latest"}
+          - {os: ubuntu-18.04, r: 'devel', rspm: "https://packagemanager.rstudio.com/cran/__linux__/bionic/latest"}
 
     env:
       R_REMOTES_NO_ERRORS_FROM_WARNINGS: true
-      CRAN: ${{ matrix.config.cran }}
+      RSPM: ${{ matrix.config.rspm }}
 
     steps:
       - uses: actions/checkout@v1
@@ -77,6 +76,16 @@ jobs:
         run: rcmdcheck::rcmdcheck(args = "--no-manual", error_on = "warning", check_dir = "check")
         shell: Rscript {0}
 
+      - name: Show install output
+        if: always()
+        run: find check -name '00install.out*' -exec cat '{}' \; || true
+        shell: bash
+
+      - name: Show testthat output
+        if: always()
+        run: find check -name 'testthat.Rout*' -exec cat '{}' \; || true
+        shell: bash
+
       - name: Upload check results
         if: failure()
         uses: actions/upload-artifact@master

.github/workflows/pkgdown.yaml

@@ -30,7 +30,6 @@ jobs:
 
       - name: Install dependencies
         run: |
-          install.packages("remotes")
           remotes::install_deps(dependencies = TRUE)
           remotes::install_dev("pkgdown")
         shell: Rscript {0}
@@ -39,5 +38,7 @@ jobs:
         run: R CMD INSTALL .
 
       - name: Deploy package
-        run: pkgdown::deploy_to_branch(new_process = FALSE)
-        shell: Rscript {0}
+        run: |
+          git config --local user.email "[email protected]"
+          git config --local user.name "GitHub Actions"
+          Rscript -e 'pkgdown::deploy_to_branch(new_process = FALSE)'

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: s2
 Title: Spherical Geometry Operators Using the S2 Geometry Library
-Version: 1.0.1.9000
+Version: 1.0.2
 Authors@R: c(
      person(given = "Dewey",
            family = "Dunnington",

NEWS.md

@@ -1,4 +1,6 @@
-# s2 (development version)
+# s2 1.0.2
+
+* Fixed CRAN check errors (#71, #75, #72).
 
 # s2 1.0.1
 

R/s2-geography.R

@@ -9,7 +9,7 @@
 #' of most functions in the s2 package so that you can use other objects with
 #' an unambiguious interpretation as a geography vector. Geography vectors
 #' have a minimal [vctrs][vctrs::vctrs-package] implementation, so you can
-#' use these objects in tibble and other packages that use the vctrs
+#' use these objects in tibble, dplyr, and other packages that use the vctrs
 #' framework.
 #'
 #' @param x An object that can be converted to an s2_geography vector

data-raw/update-s2.R

@@ -81,8 +81,17 @@ print_next <- function() {
   )
   cli::cat_bullet(
     "Fix zero-length array warnings under -Wpedantic by inserting __extension__ at the beginning ",
-    "of expressions declaring them (s2region_coverer.h#251, util/gtl/compact_array.h#124)"
+    "of expressions declaring them (s2region_coverer.h#271)"
   )
+  cli::cat_bullet(
+    "Fix compact_array zero-length array warning by disabling inline elements on gcc ",
+    "(util/gtl/compact_array.h#89)"
+  )
+  cli::cat_bullet(
+    "Fix sanitizer error for compact_array when increasing the size of a zero-length array ",
+    "by wrapping util/gtl/compact_array.h#396-397 with if (old_capacity > 0) {...}"
+  )
+
   cli::cat_bullet("Remove extra semi-colon at s2boolean_operation.h#376")
   cli::cat_bullet("Remove extra semi-colons because of FROMHOST_TYPE_MAP macro (utils/endian/endian.h#565)")
   cli::cat_bullet(

inst/include/s2/s2region_coverer.h

@@ -18,6 +18,8 @@
 #ifndef S2_S2REGION_COVERER_H_
 #define S2_S2REGION_COVERER_H_
 
+#include <cstddef>
+#include <new>
 #include <queue>
 #include <utility>
 #include <vector>
@@ -245,9 +247,27 @@ class S2RegionCoverer {
 
  private:
   struct Candidate {
+    void* operator new(std::size_t size, std::size_t max_children) {
+      return ::operator new (size + max_children * sizeof(Candidate *));
+    }
+
+    void operator delete(void* p) {
+      ::operator delete (p);
+    }
+
+    Candidate(const S2Cell& cell, const std::size_t max_children)
+        : cell(cell), is_terminal(max_children == 0) {
+      std::fill_n(&children[0], max_children,
+                  absl::implicit_cast<Candidate*>(nullptr));
+    }
+
+    // Default destructor is fine; Candidate* is trivially destructible.
+    // children must be deleted by DeleteCandidate.
+    ~Candidate() = default;
+
     S2Cell cell;
     bool is_terminal;        // Cell should not be expanded further.
-    int num_children;        // Number of children that intersect the region.
+    int num_children = 0;    // Number of children that intersect the region.
     __extension__ Candidate* children[0];  // Actual size may be 0, 4, 16, or 64 elements.
   };
 

inst/include/s2/util/gtl/compact_array.h

@@ -86,7 +86,10 @@ class compact_array_base {
 #endif
 
   // Opportunistically consider allowing inlined elements.
-#if defined(_LP64) && defined(__GNUC__)
+  // dd: this has to be disabled to pass CRAN checks, since there is a
+  // (potentially) zero-length array that is not the last element of the class (so
+  // this can't be silenced using __extension__)
+#if defined(_LP64) && defined(__GNUC__) && false
   // With 64-bit pointers, our approach is to form a 16-byte struct:
   //   [5 bytes for size, capacity, is_exponent and is_inlined]
   //   [3 bytes of padding or inlined elements]
@@ -393,8 +396,13 @@ class compact_array_base {
     value_allocator_type allocator;
 
     T* new_ptr = allocator.allocate(capacity());
-    memcpy(new_ptr, Array(), old_capacity * sizeof(T));
-    allocator.deallocate(Array(), old_capacity);
+    // dd: this modification fixes a ASAN/UBSAN error, because
+    // when old_capacity is 0, Array() is nullptr, which is UB
+    // for memcpy.
+    if (old_capacity > 0) {
+      memcpy(new_ptr, Array(), old_capacity * sizeof(T));
+      allocator.deallocate(Array(), old_capacity);
+    }
 
     SetArray(new_ptr);
   }

man/as_s2_geography.Rd

@@ -106,21 +106,9 @@ S2RegionCoverer::Candidate* S2RegionCoverer::NewCandidate(const S2Cell& cell) {
       }
     }
   }
-  size_t children_size = 0;
-  if (!is_terminal) {
-    children_size = sizeof(Candidate*) << max_children_shift();
-  }
-  Candidate* candidate = static_cast<Candidate*>(
-      ::operator new(sizeof(Candidate) + children_size));
-  candidate->cell = cell;
-  candidate->is_terminal = is_terminal;
-  candidate->num_children = 0;
-  if (!is_terminal) {
-    std::fill_n(&candidate->children[0], 1 << max_children_shift(),
-                absl::implicit_cast<Candidate*>(nullptr));
-  }
   ++candidates_created_counter_;
-  return candidate;
+  const std::size_t max_children = is_terminal ? 0 : 1 << max_children_shift();
+  return new (max_children) Candidate(cell, max_children);
 }
 
 void S2RegionCoverer::DeleteCandidate(Candidate* candidate,
@@ -129,7 +117,7 @@ void S2RegionCoverer::DeleteCandidate(Candidate* candidate,
     for (int i = 0; i < candidate->num_children; ++i)
       DeleteCandidate(candidate->children[i], true);
   }
-  ::operator delete(candidate);
+  delete candidate;
 }
 
 int S2RegionCoverer::ExpandChildren(Candidate* candidate,