Further expansions of check_syntax

check_syntax

@@ -16,19 +16,40 @@
 # For more details, see http://www.mrtrix.org/.
 
 DIFF_MODE=0
+PATHS=()
 while [[ $# -gt 0 ]]; do
   case $1 in
     --diff)
       DIFF_MODE=1
       shift
       ;;
-    *)
+    -*)
       echo "Unknown option: $1" >&2
       exit 1
       ;;
+    *)
+      PATHS+=("$1")
+      shift
+      ;;
   esac
 done
 
+if [[ $DIFF_MODE -eq 1 ]] && [[ ${#PATHS[@]} -gt 0 ]]; then
+  echo "Error: --diff and file paths cannot be specified together" >&2
+  exit 1
+fi
+
+for p in "${PATHS[@]}"; do
+  if [[ ! -f "$p" ]]; then
+    echo "Error: File does not exist: $p" >&2
+    exit 1
+  fi
+  if [[ "$p" != *.h ]] && [[ "$p" != *.cpp ]]; then
+    echo "Error: File does not have .h or .cpp extension: $p" >&2
+    exit 1
+  fi
+done
+
 LOG=syntax.log
 echo -n "Checking syntax... "
 echo "Checking syntax..." > $LOG
@@ -155,6 +176,14 @@ run_checks() {
     $grep -Po '\busing\snamespace\sstd\s?;'
   )
 
+# - Check for any instances of "if (!X.rows())" or "if (X.rows() == 0)"
+#   (or ".cols()" / ".size()" instead of ".rows()");
+#   this could be where std::optional would be appropriate
+  res="$res"$(
+    cat .check_syntax4.tmp | \
+    $grep -Po 'if\s*\((\!\w+(\(\))?((\-\>|\.)\w+(\(\))?)+\.(rows|cols|size)\(\)|\w+(\(\))?((\-\>|\.)\w+(\(\))?)+\.(rows|cols|size)\(\)\s*==\s*0\))'
+  )
+
 # - Detect any instances of C-style casts
   res="$res"$(
     cat .check_syntax4.tmp | \
@@ -167,7 +196,21 @@ run_checks() {
     $grep -Po '\b(NAN|INFINITY|NULL)\b'
   )
 
-# detect any instances of std::abs() or non-disambiguated abs() (outside of the SFINAE call in core/types.h):
+# - Detect usage of C-style "new" allocation or "delete" deallocation
+#   Only applies to cpp/cmd/ and cpp/core/;
+#   the Qt code relies heavily on naked pointers
+  if [[ ! "$f" == "cpp/gui/"* ]]; then
+    res="$res"$(
+      cat .check_syntax4.tmp | \
+      $grep -Po '(return\s+|\w+([\(\[][\d\w]*[\)\]])?\s*=\s*|\w+([\(\[][\d\w]*[\)\]])?\.reset\s*\(|\w+\(\s*|\w+<\w+\[\]>\s?\(\s?)new\s+\w+(\(|\[)?'
+    )
+    res="$res"$(
+      cat .check_syntax4.tmp | \
+      $grep -Po '(?<!=)\s*\bdelete\b\s?(\[\]|)\s*\w+[^;];'
+    )
+  fi
+
+# - Detect any instances of std::abs() or non-disambiguated abs() (outside of the SFINAE call in core/types.h):
   if [[ ! "$f" -ef "cpp/core/types.h" ]]; then
     res="$res"$(
       cat .check_syntax4.tmp | \
@@ -190,8 +233,7 @@ if [[ $DIFF_MODE -eq 1 ]]; then
   staged_cpp_files=$(git diff --cached --name-only | \
     $grep -E '\.(h|cpp)$' | \
     $grep -v '_moc\.cpp' | \
-    $grep -v 'gl_core_3_3' | \
-    $grep '^cpp/')
+    $grep -v 'gl_core_3_3')
 
   if [[ -z "$staged_cpp_files" ]]; then
     echo "OK"
@@ -216,6 +258,15 @@ if [[ $DIFF_MODE -eq 1 ]]; then
 
   rm -f "$diff_tmp"
 
+elif [[ ${#PATHS[@]} -gt 0 ]]; then
+
+  for f in "${PATHS[@]}"; do
+    cat "$f" | \
+    $grep -v '\/\/.*\bcheck_syntax\soff' > .check_syntax1.tmp
+
+    run_checks "$f"
+  done
+
 else
 
   for f in $(find cpp -type f -name '*.h' -o -name '*.cpp' | $grep -v '_moc.cpp' | $grep -v 'gl_core_3_3' | sort); do
@@ -248,6 +299,7 @@ else
   - Replace all instances of %zu in print() calls with PRI_SIZET;
   - Use nullptr rather than NULL macro;
   - Use std::numeric_limits<> or MR::NaN(F) / MR::Inf(F) rather than macros NAN / INFINITY;
-  - Do not use C-style arrays (replace with eg. std::array<>)." >> $LOG
+  - Do not use C-style arrays (replace with eg. std::array<>);
+  - Use std::optional<> rather than checks for empty matrices." >> $LOG
   exit 1
 fi

cpp/cmd/amp2response.cpp

@@ -235,7 +235,7 @@ void run() {
       volumes.push_back(all_volumes(dirs_azin.size()));
     } else {
       auto grad = DWI::get_DW_scheme(header);
-      shells.reset(new DWI::Shells(grad));
+      shells = std::make_unique<DWI::Shells>(grad);
       shells->select_shells(false, false, false);
       for (size_t i = 0; i != shells->count(); ++i) {
         volumes.push_back((*shells)[i].get_volumes());

cpp/cmd/connectomestats.cpp

@@ -194,18 +194,18 @@ void run() {
     auto opt = get_options("threshold");
     if (opt.empty())
       throw Exception("For NBS algorithm, -threshold option must be provided");
-    enhancer.reset(new MR::Connectome::Enhance::NBS(num_nodes, opt[0][0]));
+    enhancer = std::make_shared<MR::Connectome::Enhance::NBS>(num_nodes, opt[0][0]);
   } break;
   case Algorithm::TFNBS: {
-    std::shared_ptr<Stats::TFCE::EnhancerBase> base(new MR::Connectome::Enhance::NBS(num_nodes));
-    enhancer.reset(new Stats::TFCE::Wrapper(base));
+    auto base = std::make_shared<MR::Connectome::Enhance::NBS>(num_nodes);
+    enhancer = std::make_shared<Stats::TFCE::Wrapper>(base);
     load_tfce_parameters(*(dynamic_cast<Stats::TFCE::Wrapper *>(enhancer.get())));
     if (!get_options("threshold").empty())
       WARN(MR::Enum::lowercase_name(Algorithm::TFNBS) + " is a threshold-free algorithm;" + //
            " -threshold option ignored");                                                   //
   } break;
   case Algorithm::None: {
-    enhancer.reset(new MR::Connectome::Enhance::PassThrough());
+    enhancer = std::make_shared<MR::Connectome::Enhance::PassThrough>();
     if (!get_options("threshold").empty())
       WARN("No enhancement algorithm being used; -threshold option ignored");
   } break;
@@ -256,8 +256,8 @@ void run() {
 
   // Load variance groups
   auto variance_groups = GLM::load_variance_groups(design.rows());
-  const index_type num_vgs = variance_groups.size() == 0 ? 1 : (variance_groups.maxCoeff() + 1);
-  if (num_vgs > 1)
+  const index_type num_vgs = variance_groups.has_value() ? (variance_groups->maxCoeff() + 1) : 1;
+  if (variance_groups.has_value())
     CONSOLE("Number of variance groups: " + str(num_vgs));
 
   // Load hypotheses
@@ -334,15 +334,15 @@ void run() {
   // Construct the class for performing the initial statistical tests
   std::unique_ptr<GLM::TestBase> glm_test;
   if (variable_design_matrix) {
-    if (variance_groups.size() > 0)
+    if (variance_groups.has_value())
       glm_test = std::make_unique<GLM::TestVariableHeteroscedastic>(
-          data, design, hypotheses, variance_groups, extra_columns, nans_in_data, nans_in_columns);
+          data, design, hypotheses, *variance_groups, extra_columns, nans_in_data, nans_in_columns);
     else
       glm_test = std::make_unique<GLM::TestVariableHomoscedastic>(
           data, design, hypotheses, extra_columns, nans_in_data, nans_in_columns);
   } else {
-    if (variance_groups.size() > 0)
-      glm_test = std::make_unique<GLM::TestFixedHeteroscedastic>(data, design, hypotheses, variance_groups);
+    if (variance_groups.has_value())
+      glm_test = std::make_unique<GLM::TestFixedHeteroscedastic>(data, design, hypotheses, *variance_groups);
     else
       glm_test = std::make_unique<GLM::TestFixedHomoscedastic>(data, design, hypotheses);
   }

cpp/cmd/dirsplit.cpp

@@ -60,7 +60,8 @@ class Shared {
         subset(num_subsets),
         best_energy(std::numeric_limits<value_type>::max()),
         target_num_permutations(target_num_permutations),
-        num_permutations(0) {
+        num_permutations(0),
+        progress("distributing directions", target_num_permutations) {
     size_t s = 0;
     for (ssize_t n = 0; n < directions.rows(); ++n) {
       subset[s++].push_back(n);
@@ -78,15 +79,13 @@ class Shared {
 
   bool update(value_type energy, const std::vector<std::vector<size_t>> &set) {
     std::lock_guard<std::mutex> lock(mutex);
-    if (!progress)
-      progress.reset(new ProgressBar("distributing directions", target_num_permutations));
     if (energy < best_energy) {
       best_energy = energy;
       best_subset = set;
-      progress->set_text("distributing directions (current best configuration: energy = " + str(best_energy) + ")");
+      progress.set_text("distributing directions (current best configuration: energy = " + str(best_energy) + ")");
     }
     ++num_permutations;
-    ++(*progress);
+    ++progress;
     return num_permutations < target_num_permutations;
   }
 
@@ -106,7 +105,7 @@ class Shared {
   value_type best_energy;
   const size_t target_num_permutations;
   size_t num_permutations;
-  std::unique_ptr<ProgressBar> progress;
+  ProgressBar progress;
 };
 
 class EnergyCalculator {

cpp/cmd/dwiextract.cpp

@@ -111,7 +111,7 @@ void run() {
   auto opt = get_options("pe");
   const auto pe_scheme = Metadata::PhaseEncoding::get_scheme(header_in);
   if (!opt.empty()) {
-    if (!pe_scheme.rows())
+    if (!pe_scheme.has_value())
       throw Exception("Cannot filter volumes by phase-encoding: No such information present");
     const auto filter = parse_floats(opt[0][0]);
     if (!(filter.size() == 3 || filter.size() == 4))
@@ -120,13 +120,13 @@ void run() {
     for (const auto i : volumes) {
       bool keep = true;
       for (size_t axis = 0; axis != 3; ++axis) {
-        if (pe_scheme(i, axis) != filter[axis]) {
+        if ((*pe_scheme)(i, axis) != filter[axis]) {
           keep = false;
           break;
         }
       }
       if (filter.size() == 4) {
-        if (std::fabs(pe_scheme(i, 3) - filter[3]) > 5e-3)
+        if (std::fabs((*pe_scheme)(i, 3) - filter[3]) > 5e-3)
           keep = false;
       }
       if (keep)
@@ -149,12 +149,12 @@ void run() {
   Eigen::MatrixXd new_grad(volumes.size(), grad.cols());
   for (size_t i = 0; i < volumes.size(); i++)
     new_grad.row(i) = grad.row(volumes[i]);
-  DWI::set_DW_scheme(header_out, new_grad);
+  DWI::set_DW_scheme(header_out.keyval(), new_grad);
 
-  if (pe_scheme.rows()) {
-    Eigen::MatrixXd new_scheme(volumes.size(), pe_scheme.cols());
+  if (pe_scheme.has_value()) {
+    Eigen::MatrixXd new_scheme(volumes.size(), pe_scheme->cols());
     for (size_t i = 0; i != volumes.size(); ++i)
-      new_scheme.row(i) = pe_scheme.row(volumes[i]);
+      new_scheme.row(i) = pe_scheme->row(volumes[i]);
     Metadata::PhaseEncoding::set_scheme(header_out.keyval(), new_scheme);
   }
 

cpp/cmd/dwirecon.cpp

@@ -15,6 +15,7 @@
  */
 
 #include <limits>
+#include <optional>
 #include <set>
 #include <string>
 #include <vector>
@@ -464,7 +465,7 @@ void run_combine_pairs(Image<float> &dwi_in, const scheme_type &grad_in, const s
   DEBUG("Input to output index:\n" + ss_in2out.str());
 
   header_out.size(3) = dwi_in.size(3) / 2;
-  DWI::set_DW_scheme(header_out, grad_out);
+  DWI::set_DW_scheme(header_out.keyval(), grad_out);
   Image<float> dwi_out = Image<float>::create(header_out.name(), header_out);
 
   if (field_image.valid()) {
@@ -923,6 +924,9 @@ void run() {
   auto dwi_in = Header::open(argument[0]).get_image<float>();
   auto grad_in = DWI::get_DW_scheme(dwi_in);
   auto pe_in = Metadata::PhaseEncoding::get_scheme(dwi_in);
+  if (!pe_in.has_value())
+    throw Exception("All currently-implemented reconstruction algorithms require a phase encoding table,"
+                    " but none was found");
 
   Header header_out(dwi_in);
   header_out.datatype() = DataType::Float32;
@@ -933,7 +937,7 @@ void run() {
 
   case Operation::COMBINE_PAIRS:
     Metadata::PhaseEncoding::clear_scheme(header_out.keyval());
-    run_combine_pairs(dwi_in, grad_in, pe_in, header_out);
+    run_combine_pairs(dwi_in, grad_in, *pe_in, header_out);
     break;
 
   case Operation::COMBINE_PREDICTED:
@@ -943,7 +947,7 @@ void run() {
     // - Fraction of each sample that comes from its own empirical data vs. prediction
     // - The predicted intensity generated there
     // Also throw in a bunch of assertions to make sure that the prediction generation is working
-    run_combine_predicted(dwi_in, grad_in, pe_in, header_out);
+    run_combine_predicted(dwi_in, grad_in, *pe_in, header_out);
     break;
 
   default: // no others yet implemented

cpp/cmd/fixelcfestats.cpp

@@ -306,8 +306,8 @@ void run() {
 
   // Load variance groups
   auto variance_groups = Math::Stats::GLM::load_variance_groups(design.rows());
-  const Math::Stats::index_type num_vgs = variance_groups.size() ? variance_groups.maxCoeff() + 1 : 1;
-  if (num_vgs > 1)
+  const Math::Stats::index_type num_vgs = variance_groups.has_value() ? (variance_groups->maxCoeff() + 1) : 1;
+  if (variance_groups.has_value())
     CONSOLE("Number of variance groups: " + str(num_vgs));
 
   // Load hypotheses
@@ -443,16 +443,16 @@ void run() {
   // Construct the class for performing the initial statistical tests
   std::unique_ptr<Math::Stats::GLM::TestBase> glm_test;
   if (variable_design_matrix) {
-    if (variance_groups.size())
+    if (variance_groups.has_value())
       glm_test = std::make_unique<Math::Stats::GLM::TestVariableHeteroscedastic>(
-          data, design, hypotheses, variance_groups, extra_columns, nans_in_data, nans_in_columns);
+          data, design, hypotheses, *variance_groups, extra_columns, nans_in_data, nans_in_columns);
     else
       glm_test = std::make_unique<Math::Stats::GLM::TestVariableHomoscedastic>(
           data, design, hypotheses, extra_columns, nans_in_data, nans_in_columns);
   } else {
-    if (variance_groups.size())
+    if (variance_groups.has_value())
       glm_test =
-          std::make_unique<Math::Stats::GLM::TestFixedHeteroscedastic>(data, design, hypotheses, variance_groups);
+          std::make_unique<Math::Stats::GLM::TestFixedHeteroscedastic>(data, design, hypotheses, *variance_groups);
     else
       glm_test = std::make_unique<Math::Stats::GLM::TestFixedHomoscedastic>(data, design, hypotheses);
   }

cpp/cmd/fixelfilter.cpp

@@ -172,7 +172,7 @@ void run() {
       const value_type cfe_h = get_option_value("cfe_h", Fixel::Filter::cfe_default_h);
       const value_type cfe_c = get_option_value("cfe_c", Fixel::Filter::cfe_default_c);
       const bool cfe_legacy = get_options("cfe_legacy").size();
-      filter.reset(new Fixel::Filter::CFE(matrix, cfe_dh, cfe_e, cfe_h, cfe_c, !cfe_legacy));
+      filter = std::make_unique<Fixel::Filter::CFE>(matrix, cfe_dh, cfe_e, cfe_h, cfe_c, !cfe_legacy);
       option_list.erase("cfe_dh");
       option_list.erase("cfe_e");
       option_list.erase("cfe_h");
@@ -184,7 +184,7 @@ void run() {
       const float connect =
           get_option_value("threshold_connectivity", Fixel::Filter::Connect::default_connectivity_threshold);
       // TODO What does / should -mask do here?
-      filter.reset(new Fixel::Filter::Connect(matrix, value, connect));
+      filter = std::make_unique<Fixel::Filter::Connect>(matrix, value, connect);
       output_header.datatype() = DataType::UInt32;
       output_header.datatype().set_byte_order_native();
       option_list.erase("threshold_value");
@@ -193,7 +193,7 @@ void run() {
     case FilterType::SMOOTH: {
       const float fwhm = get_option_value("fwhm", Fixel::Filter::Smooth::default_fwhm);
       const float threshold = get_option_value("minweight", Fixel::Filter::Smooth::default_threshold);
-      filter.reset(new Fixel::Filter::Smooth(index_image, matrix, fwhm, threshold));
+      filter = std::make_unique<Fixel::Filter::Smooth>(index_image, matrix, fwhm, threshold);
       option_list.erase("fwhm");
       option_list.erase("minweight");
     } break;

cpp/cmd/meshfilter.cpp

@@ -95,7 +95,7 @@ void run() {
     const default_type spatial = get_option_value("smooth_spatial", Filter::default_smoothing_spatial_factor);
     const default_type influence = get_option_value("smooth_influence", Filter::default_smoothing_influence_factor);
     const std::string msg = in.size() > 1 ? "Applying smoothing filter to multiple meshes" : "";
-    filter.reset(new Filter::Smooth(msg, spatial, influence));
+    filter = std::make_unique<Filter::Smooth>(msg, spatial, influence);
   } else {
     assert(0);
   }