Updates to improve the Softwipe score#77
Updates to improve the Softwipe score#77trongnhanuit wants to merge 43 commits intoiqtree:masterfrom
Conversation
There was a problem hiding this comment.
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
- NULL → nullptr throughout the codebase (correct C++11 modernization)
- Braces around single-statement if/for bodies (clang-tidy style fix)
- override keyword added to virtual method overrides in headers (genuinely useful — catches future signature mismatches at compile time)
- Operator precedence parentheses e.g. part3+3+j → (part3)+3+j (cosmetic only — precedence was already correct)
- 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)
- Unused functions commented out with /* ... */ in candidateset, genomenode, constrainttree
- 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.
|
@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
|
@bqminh I've merged the latest code from the master branch. Please proceed with the next steps. Thanks. |
This pull request includes a bunch of commits to improve the Softwipe score