From 2708499874d646fa89da1ba5a5c88c76401f1b12 Mon Sep 17 00:00:00 2001 From: amkram Date: Mon, 13 Jul 2026 19:28:56 -0700 Subject: [PATCH] style: deslop comments across first-party sources Comment/docstring-only cleanup, same rubric as the earlier deslop pass but covering the code merged since (index param header, mmap uncompressed index, parallel FASTQ reader, scoring prefetch, ZSTD framing) and applied more aggressively: - delete perf-narrative / commit-message flavor from the source (malloc %, ms-overhead, L3-latency, load-factor, "hot path") -- it lives in git history; - delete comments that don't earn their place (restating obvious code, struct/section labels, decorative dividers, trailers echoing the field name); - compress every remaining >2-line comment toward the essential one or two lines; - keep every non-obvious why, invariant, unit, math derivation, citation, TODO. No behavior change. Verified comments-only vs main (comment+whitespace-stripped code byte-identical across all 22 files); build clean, 0 warnings; unit + e2e + examples pass. Excludes mgsr.* and samtools-derived pileup.c/h. 22 files, -223 lines net. --- src/conversion.cpp | 21 ++-- src/gap_map_utils.hpp | 6 +- src/genotyping.cpp | 12 +-- src/genotyping.hpp | 9 +- src/index_single_mode.cpp | 22 ++-- src/index_single_mode.hpp | 17 ++- src/logging.hpp | 31 +----- src/main.cpp | 159 ++++++++++------------------ src/panmap_utils.hpp | 12 +-- src/placement.cpp | 96 ++++++----------- src/placement.hpp | 55 ++++------ src/progress_tracker.hpp | 2 - src/seeding.cpp | 5 +- src/seeding.hpp | 20 ++-- src/test/helpers/metrics_oracle.hpp | 23 ++-- src/test/helpers/seed_helpers.hpp | 5 +- src/test/helpers/test_index.hpp | 12 +-- src/test/test_index.cpp | 48 +++------ src/test/test_placement.cpp | 64 +++-------- src/test/test_seeding.cpp | 5 +- src/zstd_compression.cpp | 21 ++-- src/zstd_compression.hpp | 18 +--- 22 files changed, 220 insertions(+), 443 deletions(-) diff --git a/src/conversion.cpp b/src/conversion.cpp index a5cfd4f3..a8852bb2 100644 --- a/src/conversion.cpp +++ b/src/conversion.cpp @@ -18,16 +18,12 @@ extern "C" { #include } -// Run a bcftools function in a forked child process. bcftools main_* use global optind -// and other process-wide state; forking gives each call its own address space for safe -// parallelism. +// Run a bcftools function in a forked child. bcftools main_* use global optind and other +// process-wide state; forking gives each call its own address space for safe parallelism. static int run_bcftools_in_fork(int (*func)(int, char**), int argc, char** argv, bool silenceStderr = true) { - // Serialize the forks. fork() from a TBB worker (batch mode, --threads > 1) - // clones only the calling thread; if another worker holds a stdio/htslib lock - // at fork time, the child can deadlock acquiring it (the owner doesn't exist in - // the child). glibc/macOS already make malloc fork-safe via atfork handlers, so - // the remaining exposure is two bcftools children running concurrently. This - // lock allows at most one bcftools fork in flight at a time. + // Serialize forks: fork() from a TBB worker clones only the calling thread, so a lock + // another worker holds (stdio/htslib) can deadlock the child. This mutex bounds it to one + // bcftools fork in flight (malloc is already fork-safe via atfork). static std::mutex forkMutex; std::lock_guard forkLock(forkMutex); @@ -313,11 +309,8 @@ static bam1_t* build_bam_from_result(const std::string& qname_full, qname.resize(qname.size() - 2); } - // R2 of a pair is reverse-complemented upstream (readFastqPaired) before alignment, - // so the aligner's rev bit for it is inverted relative to the original fragment. - // Report the original read's true strand in the FLAG and TLEN; the stored SEQ/CIGAR - // stay in the aligned (forward-reference) orientation, which is what BAM expects for - // either strand. + // R2 was reverse-complemented upstream, so the aligner's rev bit is inverted vs the original + // fragment. Report true strand in FLAG/TLEN; SEQ/CIGAR stay forward-reference, as BAM expects. const uint8_t effective_rev = (is_paired && !is_read1) ? static_cast(!aln->rev) : aln->rev; uint16_t flag = compute_sam_flags(is_paired, is_read1, effective_rev, mate_rev, proper_frag, mate_unmapped); diff --git a/src/gap_map_utils.hpp b/src/gap_map_utils.hpp index 8eb2515c..b4fc5201 100644 --- a/src/gap_map_utils.hpp +++ b/src/gap_map_utils.hpp @@ -277,10 +277,8 @@ void invertGapMap(std::map& gapMap, // start inside of a range auto curIt = leftIt; if (static_cast(end) <= static_cast(curIt->second)) { - // [start, end] lies entirely within one gap run; reversing an all-gap span - // is a no-op. Without this clamp, the walk below appended a run to the gap's - // end plus a negative-length run, corrupting the gap map (e.g. inverting - // [10,20] inside {0:30} dropped [21,30]). + // [start,end] within one gap run: reversing an all-gap span is a no-op; without this + // clamp the walk emits a negative-length run (inverting [10,20] inside {0:30} dropped [21,30]). blockRuns.emplace_back(true, std::make_pair(static_cast(start), static_cast(end))); } else { diff --git a/src/genotyping.cpp b/src/genotyping.cpp index c8da322f..3698af06 100644 --- a/src/genotyping.cpp +++ b/src/genotyping.cpp @@ -245,10 +245,8 @@ std::string genotyping::applyMutationSpectrum(const std::string& line, gls[0] += scaled_submat[ref_nuc_idx][ref_nuc_idx]; for (size_t i = 1; i < gls.size(); i++) { - // scaled_submat is the 4x4 A/C/G/T substitution-spectrum prior, so apply - // it only to base ALTs. A non-base ALT (e.g. '*' spanning deletion) has - // index > 3 and would over-read the row, so leave its likelihood - // unmodified. Matches how indels use the unmodified bcftools posterior. + // scaled_submat is the 4x4 A/C/G/T substitution prior; apply only to base ALTs. A + // non-base ALT (e.g. '*') has index >3 and would over-read the row, so leave it as-is. int alt_idx = getIndexFromNucleotide(alts[i - 1]); if (alt_idx <= 3) { gls[i] = gls[i] + scaled_submat[ref_nuc_idx][alt_idx]; @@ -264,9 +262,9 @@ std::string genotyping::applyMutationSpectrum(const std::string& line, if (min_gl_index == 0) return ""; - // Consensus gate: only emit an ALT that is the majority allele and has at least - // minDepth high-quality reads. Guards against bcftools (and, at low coverage, the - // spectrum prior) emitting sub-majority or shallow ALTs as false positives. + // Consensus gate: emit an ALT only if it's the majority allele with >= minDepth + // high-quality reads. Guards against bcftools / the low-coverage spectrum prior + // emitting sub-majority or shallow false-positive ALTs. if (!passesConsensusGate(min_gl_index, ads, minDepth)) return ""; gt = min_gl_index; diff --git a/src/genotyping.hpp b/src/genotyping.hpp index 842b19b7..09f0efda 100644 --- a/src/genotyping.hpp +++ b/src/genotyping.hpp @@ -34,11 +34,10 @@ void fillMutationMatricesFromFile(mutationMatrices& mutMat, std::ifstream& inf); std::string applyMutationSpectrum(const std::string& line, const std::vector>& scaled_submat, int minDepth, double minQual); -// Field-standard consensus gate for haploid genotyping: the called allele (calledIdx -// into AD; 0 = ref) must (a) be the majority — strictly more high-quality reads than -// all other alleles combined — and (b) be supported by at least minDepth high-quality -// reads. Below that, a position should be a no-call (ref/N), not a variant. Returns -// true (no filtering) only when AD is missing/uninformative. +// Consensus gate for haploid genotyping: the called allele (calledIdx into AD; 0 = ref) +// must be the majority (strictly more high-quality reads than all others combined) and +// have >= minDepth high-quality reads, else the position is a no-call. Returns true (no +// filtering) only when AD is missing/uninformative. bool passesConsensusGate(int calledIdx, const std::vector& ad, int minDepth); // Same gate applied to a raw VCF sample field ("GT:PL:AD"). bool passesConsensusGate(const std::string& sampleField, int minDepth); diff --git a/src/index_single_mode.cpp b/src/index_single_mode.cpp index 6497ca9f..b55d6f89 100644 --- a/src/index_single_mode.cpp +++ b/src/index_single_mode.cpp @@ -345,9 +345,8 @@ std::vector index_single_mode::IndexBuilder::compu localRangeCoordToGlobalScalarCoords.push_back(curCoordScalar); localRangeCoordToBlockId.push_back(curCoord.primaryBlockId); } else if (refOnSyncmers.contains(curCoordScalar)) { - // Only delete seeds inside the genome extent. Flank regions (before - // firstNonGapScalar or after lastNonGapScalar) are missing data, not true - // gaps; their seeds stay inherited from the parent. + // Only delete seeds inside the genome extent; flank regions are missing + // data, not true gaps, so their seeds stay inherited from the parent. if (std::cmp_greater_equal(curCoordScalar, firstNonGapScalar) && std::cmp_less_equal(curCoordScalar, lastNonGapScalar)) { blockOnSyncmers[curCoord.primaryBlockId].erase(curCoordScalar); @@ -368,9 +367,8 @@ std::vector index_single_mode::IndexBuilder::compu recomputeBlock = false; recomputeInProgress = false; if (curCoordGapMapIt != gapMap.end()) { - // At begin() there is no previous run, so the lower bound - // (prev->second < curCoordScalar) is vacuously satisfied; - // guard the std::prev to avoid dereferencing before begin(). + // At begin() there is no previous run (lower bound vacuously + // satisfied); guard std::prev to avoid dereferencing before begin(). while (curCoordGapMapIt != gapMap.end() && !((curCoordGapMapIt == gapMap.begin() || std::cmp_less(std::prev(curCoordGapMapIt)->second, curCoordScalar)) && @@ -1386,11 +1384,8 @@ void index_single_mode::IndexBuilder::computeSubstitutionSpectrum() { uint32_t blockId = blockMut.primaryBlockId; bool oldExists = blockSequences.blockExists[blockId]; blockUndoRecord.emplace_back(blockId, oldExists); - // Match the canonical block-existence rule (panmap_utils.cpp): an - // insertion makes the block exist (even when also inverted); a - // non-inversion deletion removes it; an inversion of an existing - // block leaves existence unchanged. blockMutInfo=false marks both - // deletions and inversions, so it can't be assigned directly. + // Canonical block-existence rule (panmap_utils.cpp): insertion sets exists, non-inversion + // deletion clears it, inversion leaves it unchanged; blockMutInfo=false marks del+inv both. if (blockMut.blockMutInfo) { blockSequences.blockExists[blockId] = true; } else if (!blockMut.inversion) { @@ -2367,9 +2362,8 @@ void index_single_mode::IndexBuilder::buildIndexParallel(int numThreads) { totalClonesCreated_.fetch_add(1, std::memory_order_relaxed); - // Walk from root to the range's first node, then DFS through the range. - // Nodes 0..startDfs-1 are ancestors or earlier siblings that must be walked. - // Compute nodeChanges only for nodes in [startDfs, endDfs). + // Walk from root to the range's first node, then DFS through the range. Compute + // nodeChanges only for nodes in [startDfs, endDfs); earlier nodes are just walked. std::function processDfsRange = [&](panmanUtils::Node* node, uint64_t dfsIdx) { diff --git a/src/index_single_mode.hpp b/src/index_single_mode.hpp index 9b686410..6c6d2c89 100644 --- a/src/index_single_mode.hpp +++ b/src/index_single_mode.hpp @@ -21,19 +21,16 @@ namespace index_single_mode { -// A small uncompressed header prepended to the .idx so cache-validation can read the -// seeding params without decompressing the (up to GB-scale) payload -- previously the -// validation step fully decompressed the index just to read six fields, then the -// placement step decompressed it a second time. +// Uncompressed header prepended to the .idx so cache-validation can read the seeding +// params without decompressing the (up to GB-scale) payload. constexpr uint32_t kIndexMagic = 0x31494D50u; // "PMI1" little-endian constexpr uint32_t kIndexHeaderVersion = 1; constexpr size_t kIndexHeaderSize = 32; struct IndexParamsHeader { int32_t k = 0, s = 0, t = 0, l = 0; bool hpc = false, open = false; - // When set, the payload after the header is raw (unframed) capnp bytes rather than - // ZSTD frames, so load can mmap it and hand the bytes straight to capnp (zero-copy, - // no decompression pass). Bigger on disk, faster to load. + // When set, payload is raw (unframed) capnp bytes, not ZSTD frames, so load can mmap + // and hand them straight to capnp (zero-copy). Bigger on disk, faster to load. bool uncompressed = false; }; std::array encodeIndexHeader(const IndexParamsHeader& p); @@ -69,10 +66,8 @@ struct BuildState { std::map gapMap; std::unordered_set invertedBlocks; - // first/last non-gap scalar positions. Seeds in flank regions (before - // firstNonGapScalar or after lastNonGapScalar) are missing data, not true gaps, - // so must not be deleted when they become gaps. Permissive by default; only - // restricted when extentGuard is on. + // first/last non-gap scalar positions. Seeds in flank regions are missing data, not + // true gaps, so must not be deleted when they become gaps; permissive unless extentGuard is on. uint64_t firstNonGapScalar = 0; uint64_t lastNonGapScalar = UINT64_MAX; diff --git a/src/logging.hpp b/src/logging.hpp index 5d679572..73929e05 100644 --- a/src/logging.hpp +++ b/src/logging.hpp @@ -106,7 +106,6 @@ inline const char* cross() { } } // namespace box -// Spinner frames (Braille pattern, 10 frames at ~80ms = ~12fps) inline const char* spinner_frame(int i) { static const char* frames[] = {"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"}; static const char* plain_frames[] = {"|", "/", "-", "\\", "|", "/", "-", "\\", "|", "/"}; @@ -165,8 +164,6 @@ inline void debug(const std::string& msg) { std::cerr << style::dim() << msg << style::reset() << "\n"; } -// Buffered single-line dim stream for routine log lines. detail() visible unless -// --quiet; trace() only under --verbose. class LineBuf { public: explicit LineBuf(bool enabled) : enabled_(enabled) {} @@ -181,7 +178,7 @@ class LineBuf { return *this; } - // ostream manipulators (std::endl / std::flush) flush the line immediately. + // std::endl / std::flush flush the line immediately. LineBuf& operator<<(std::ostream& (*)(std::ostream&)) { emit(); return *this; @@ -211,18 +208,13 @@ inline LineBuf trace() { return LineBuf(config().verbose && !config().quiet); } -// One-line tool+version banner. inline void banner(const std::string& version, const std::string& /*subtitle*/ = "") { if (config().quiet) return; std::cerr << style::bold() << "panmap" << style::reset() << style::dim() << " " << version << style::reset() << "\n\n"; } -// Stage line columns: -// " ICN LABEL. SUBJECT................. STAT......... TIME" -// Subject/stat are left-padded to a min width so columns align across stages; -// time is right-padded so right edges line up. Input strings must be plain text -// (no ANSI codes) or the width math breaks; apply styling via the helpers below. +// Stage-line inputs must be plain text (no ANSI) or the column width math breaks. constexpr int kLabelWidth = 6; constexpr int kSubjectWidth = 22; constexpr int kStatWidth = 14; @@ -242,8 +234,7 @@ inline std::string pad_left(const std::string& s, int w) { return std::string(w - s.size(), ' ') + s; } -// Truncate to w columns, keeping the tail (informative end of a path) behind a -// leading ellipsis. Assumes ASCII content. +// Keeps the tail behind a leading ellipsis. Assumes ASCII content. inline std::string truncate_tail(const std::string& s, int w) { if (w <= 0) return ""; if ((int)s.size() <= w) return s; @@ -270,9 +261,6 @@ inline void write_status_line(const std::string& icon, const std::string& stat, int64_t ms) { if (config().quiet) return; - // Subject column flexes to terminal width so stat/time columns stay aligned - // across rows; long subjects (paths) are tail-truncated. Non-TTY (piped/log) - // keeps the full subject and the original min width. int subjectW; if (config().isTTY) { int iconW = config().plain ? static_cast(icon.size()) : 1; @@ -312,7 +300,6 @@ inline void done(const std::string& what, int64_t ms) { << style::reset() << "\n"; } -// Trailing blank line before the shell prompt. inline void summary(int64_t /*total_ms*/) { if (config().quiet) return; std::cerr << "\n"; @@ -335,7 +322,6 @@ inline void step(const std::string& msg) { std::cerr << style::dim() << " " << msg << style::reset() << "\n"; } -// Indeterminate progress message (one line, overwritten in place). inline void progress(const std::string& msg) { if (config().quiet || !config().isTTY || config().plain) return; std::cerr << "\r\033[K" << style::dim() << " " << msg << style::reset() << std::flush; @@ -345,9 +331,6 @@ inline void progress_clear() { if (config().isTTY && !config().plain) std::cerr << "\r\033[K" << std::flush; } -// Progress bar. In-place line: -//