Skip to content

Updates to improve the Softwipe score#77

Open
trongnhanuit wants to merge 43 commits intoiqtree:masterfrom
trongnhanuit:master
Open

Updates to improve the Softwipe score#77
trongnhanuit wants to merge 43 commits intoiqtree:masterfrom
trongnhanuit:master

Conversation

@trongnhanuit
Copy link
Member

This pull request includes a bunch of commits to improve the Softwipe score

thomaskf and others added 30 commits June 26, 2025 12:33
…'+'; add parentheses to explicitly specify the order of operations" - f3754ac"
… the condition 'var' is redundant or there is possible null pointer dereference - a6fbe97"
Copy link
Member

@bqminh bqminh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code:

PR #77 Review — "Updates to improve the Softwipe score"

Summary

A large mechanical code quality PR touching 144 files. Mostly low-risk modernization, but three issues should block merging.

What It Does

  1. NULL → nullptr throughout the codebase (correct C++11 modernization)
  2. Braces around single-statement if/for bodies (clang-tidy style fix)
  3. override keyword added to virtual method overrides in headers (genuinely useful — catches future signature mismatches at compile time)
  4. Operator precedence parentheses e.g. part3+3+j → (part3)+3+j (cosmetic only — precedence was already correct)
  5. parseArg() refactored: ~530-line inline param initialization block replaced with a call to the existing params.setDefault() (good design, but needs verification that coverage is identical)
  6. Unused functions commented out with /* ... */ in candidateset, genomenode, constrainttree
  7. New CI/CD workflow for multi-platform builds (Linux x86/ARM, macOS x86/ARM64, Windows)

Issues That Should Block Merge

1. yaml-cpp/src/null.cpp — behavioral regression
Added: || str == "nullptr"
"nullptr" is a C++ keyword, not a YAML null literal. The YAML spec defines null as ~, null, Null, NULL, or empty. Adding "nullptr" means any YAML value or model name containing that string gets silently coerced to null. This should be reverted.

2. ASSERT on checkpoint->getString() — wrong semantics
ASSERT(checkpoint->getString("", str));
getString() returns false when a key doesn't exist in the checkpoint — which is a valid, expected condition on a fresh run. In debug builds this would abort; in release builds (-DNDEBUG) the ASSERT becomes ((void)0) and silently discards the return. The original code ignoring the return was correct.

3. using namespace std removed from tools.h — potential build breakage
tools.h is included by nearly every translation unit. Removing this transitive "using namespace std" could break compilation in .cpp files that relied on it. The CI only runs on the fork's environment, which may not catch this on all compilers.

Issues Worth Addressing

  • Dead code: Commented-out functions (addCandidateSplits, getTopologyScore, etc.) should be removed entirely or guarded with #if 0 and an explanatory comment — not left as /* ... */ blocks mixed with active code.
  • mcmc_bds default changed from "1,1,0.5" to "1 1 0.5" — this is actually a latent bug fix (the parser stores space-separated) but it's unannounced.
  • 99 commits behind upstream master: Needs rebase/merge before it can integrate cleanly.
  • Mixed indentation introduced in constrainttree.cpp (tabs vs spaces).

Overall

The nullptr modernization, override additions, and CI/CD workflow are valuable. The three blocking issues (yaml-cpp regression, ASSERT misuse, using namespace std removal) need to be fixed before merging.

@bqminh
Copy link
Member

bqminh commented Mar 5, 2026

@trongnhanuit do this before I continue: 99 commits behind upstream master: Needs rebase/merge before it can integrate cleanly.

# Conflicts:
#	alignment/alignment.cpp
#	alignment/superalignment.cpp
#	utils/tools.cpp
@trongnhanuit
Copy link
Member Author

@bqminh I've merged the latest code from the master branch. Please proceed with the next steps. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants