Pull Request №697 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/rule-of-zero-batched-fixes
Merge: d10f3af574b60550f256856d5a9742600ce1128b←4d2c33ca7a4b5531b084f002cd469d790f5fa119
Apply Rule of Zero to old-style uncopyable classes in core/ and protocols/
----------------
Merge commit message:
Apply Rule of Zero to old-style uncopyable classes in core/ and protocols/
Modernize 31 files that used the pre-C++11 idiom of declaring (but not
defining) private copy constructor and assignment operator to forbid
copies. The link-error-as-enforcement trick predates `= delete` and is
fragile (silent same-translation-unit ICE breakage, confusing diagnostics).
Two flavours of fix:
1. Stand-alone non-copyable classes (InteractionGraphBase node/edge/graph
family, FlexbbInteractionGraph, JobDigraph, AtomTreeCollection's
ResidueAtomTreeCollection, Matcher, MonteCarlo's operator=): replace
the unimplemented private declarations with public `= delete` copies.
Where the old assignment took a non-const reference or returned `T const &`
that was a holdover from the C++03 idiom; the modernized signatures take
`T const &` and return `T &`.
2. Singleton-derived factories (utility::SingletonBase children): drop the
redundant unimplemented copy/assignment declarations entirely. The base
class already `= delete`s its own copy/assignment, which transitively
makes the derived class's implicit copies deleted, so the extra lines
were noise.
No behaviour change: copies/assignments that were link errors before are
now compile errors, which is the intended improvement in diagnostic
quality. Debug build passes clean.
Pull Request №696 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/fix/incorrect-comparison-operators
Merge: d10f3af574b60550f256856d5a9742600ce1128b←453f38ad4e9ffd93c2be2fcabe2843fe3a16c9a8
Fix incorrect comparison logic in operator< / difference_from
----------------
Merge commit message:
Add comments explaining each comparison-operator fix
Brief one-paragraph notes above each of the four sites describing
the bug and why the corrected form is the right one, so a future
reader doesn't accidentally revert to the simpler-looking but
broken version.
Pull Request №696 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/fix/incorrect-comparison-operators
Merge: d10f3af574b60550f256856d5a9742600ce1128b←8fc631652510bbfa47c5575107e9de808b7804ce
Fix incorrect comparison logic in operator< / difference_from
----------------
Merge commit message:
Fix incorrect comparison logic in operator< / difference_from
Four independent classes had broken comparison logic that violated
either the contract of `operator<` (strict weak ordering) or the
intended semantics of `difference_from`:
* `core/chemical/sdf/mol_util.cc`: `BondData::operator<` was
`(lower < other.lower) || (upper < other.upper)`. This is not a
strict weak ordering — for `(5,1)` vs `(3,7)`, both compare less
than each other, breaking antisymmetry. `BondData` is used inside
`std::set<BondData>` (see `parse_bond_type_data`), so the broken
ordering directly affects the set. Replaced with a proper
lexicographic compare via `std::tie`.
* `core/scoring/motif/motif_hash_stuff.cc`:
`ResPairMotif::operator<` returned `0 < memcmp(...)`, i.e. it
reported "less than" exactly when memcmp said "greater than" —
the comparison was inverted. Switched to `memcmp(...) < 0`, which
matches the correct convention (and matches `MotifHit::operator<`
in the same file).
* `core/io/StructFileReaderOptions.cc`: `operator<` was using `==`
instead of `!=` for the short-circuit "return false" lines. The
intent of the pattern is "if a < b return true; if a != b return
false; otherwise fall through to the next member" (this is exactly
what the parent `StructFileRepOptions::operator<` does), but the
`==` form returned false when members were equal — short-circuiting
before the rest of the members were ever compared, *and* failed to
return false when members satisfied `>`. Fixed every `==` to `!=`
and added the missing `!=` follow-up after `glycam_pdb_format_`.
* `core/chemical/gasteiger/GasteigerAtomTypeData.cc`:
`difference_from` short-circuited "return Other" when
`charge_ == OTHER.charge_ || element_type_ != ... || ...`.
The first clause is inverted: the function should return Other
when these fundamental properties *differ*, not when they match.
Otherwise, two types with equal charge but otherwise comparable
properties were misclassified as `Other` and the rest of the
function (which uses `charge_` to compute lone-pair / s-orbital
/ p-orbital differences) was unreachable. Fixed `==` to `!=`.
Rosetta Benchmark 2 months Tests for this revision was automatically canceled because newer set of tests for pull-request №696 was submitted!
Pull Request №695 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/fix/off-by-one-pose-residue-loops
Merge: d10f3af574b60550f256856d5a9742600ce1128b←4b3d46d15a4d313462e17745dd631debe813c3b1
Fix off-by-one bugs in 1-indexed loops over pose residues
----------------
Merge commit message:
Fix off-by-one bugs in 1-indexed loops over pose residues
Replace `i != end` with `i <= end` (or `i != size()` with `i <= size()`)
in several loops that iterate over 1-indexed residue ranges. With the
old condition, the last residue (index == end) was skipped:
* core/pack/task/PackerTask_.cc: pymol-style debug selection output
omitted the last residue.
* protocols/forge/components/BDR.cc:
- the full-atom check used to gate `switch_to_residue_type_set`
skipped the last residue, so a non-full-atom last residue could
bypass conversion and later cause sidechain-restore mismatch.
- the loop building the `RestrictResidueToRepacking` operation
skipped the last residue, so residues outside `new_positions`
at the C-terminus were designed instead of repack-only.
* protocols/fldsgn/BluePrintBDR.cc: same full-atom check bug as BDR.
* protocols/denovo_design/components/StructureData.cc:
`compute_cutpoints` skipped the last residue, dropping a
CUTPOINT_LOWER variant on the C-terminal residue.
* protocols/enzdes/BackboneSampler.cc: the user-requested number of
trials was off by one (e.g. `bb_moves_=1000` actually ran 999).
Pull Request №694 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/utility-misc-empty-dtors
Merge: d10f3af574b60550f256856d5a9742600ce1128b←dea119ab102f68e70b46cf23cbadd6842c68bd31
Apply Rule of Zero across remaining utility/ empty destructors
----------------
Merge commit message:
Apply Rule of Zero across remaining utility/ empty destructors
Remove user-declared destructors and out-of-line `= default` definitions
where the implicit (or `= default`) destructor is correct, and clarify a
non-trivial destructor by deleting copy/move on its owner.
* Remove empty `~Foo() {}` / `~Foo() override {}` where the implicit
destructor is already correct (and virtual via base when needed):
- utility/Bound.hh, utility/excn/Exceptions.hh,
utility/io/ocstream.hh, utility/keys/AutoKey.hh,
utility/keys/UserKey.hh.
* Convert `~Foo() {}` to `~Foo() = default;` for polymorphic root
classes whose implicit destructor would otherwise be non-virtual:
- utility/Show.hh, utility/factory/WidgetFactory.hh,
utility/io/irstream.hh, utility/io/orstream.hh,
utility/keys/Key.hh, utility/options/Option.hh.
* Drop `= default` destructor pairs (.hh declaration + .cc definition)
for classes inheriting from utility::VirtualBase, which already
supplies a virtual destructor:
- utility/heap, utility/integer_mapping (subset_mapping),
utility/recent_history_queue, utility/io/GeneralFileManager
(GeneralFileContents{,Vector}), utility/tag/Tag.
* utility/io/mpistream.hh: explicitly `= delete` the copy ctor and
copy assignment of `basic_mpi_streambuf` because its destructor
calls `flush_final()` (closes the MPI channel); replace the empty
`~basic_mpi_ostream() override {}` with `= default`.
Pull Request №693 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/utility-options-empty-dtors
Merge: d10f3af574b60550f256856d5a9742600ce1128b←df1a70b0d08400256a9cb9cd9aee1fa7d0dfcf0b
Apply Rule of Zero across utility/options/ empty destructors
----------------
Merge commit message:
Apply Rule of Zero across utility/options/ empty destructors
Drop user-declared empty destructors from option and option-key classes.
The implicit destructor preserves virtual dispatch through inherited
virtual destructors (Option::~Option() and Key::~Key()).
utility/options/ — abstract bases and remaining leaf options:
AnyOption, AnyVectorOption, ScalarOption, ScalarOption_T_,
VectorOption, VectorOption_T_, PathOption, PathVectorOption,
StringOption, StringVectorOption, ResidueChainVectorOption.
utility/options/keys/ — base OptionKey plus 16 leaf key types:
Any/AnyVector, Boolean/BooleanVector, File/FileVector,
Integer/IntegerVector, Path/PathVector, Real/RealVector,
ResidueChainVector, Scalar, String/StringVector, Vector.
Follow-up to PR #692, which left these classes for a second batch.
Pull Request №692 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/utility-iterator-rule-of-zero
Merge: 80efd178b93d19b7cb76d9a0b34788104381afaa←b82b5545627e8bddf859f1c268b8e341296c449a
Apply Rule of Zero across utility/ helpers
----------------
Merge commit message:
Apply Rule of Zero across utility/ helpers
Remove user-declared destructors and trivial copy ctor/assignment
implementations whose bodies are equivalent to the compiler-generated
defaults. The implicit special members are correct in every case:
* utility/DereferenceIterator.hh: empty user dtor.
* utility/graph/ArrayPool.hh (Array0): empty dtor plus member-wise
copy ctor and self-assignment-guarded copy assignment.
* utility/graph/Graph.hh / Digraph.hh: empty dtor and = default /
member-wise copy ops on EdgeListElement, EdgeListIterator,
EdgeListConstIterator and the Directed* counterparts (non-owning
raw pointers, default copy is correct).
* utility/graph/LowMemGraph.hh (LowMemGraphBase): empty override dtor;
base utility::VirtualBase already supplies a virtual destructor.
* utility/graph/UpperEdgeGraph.hh: empty UEEdge dtor and the
~UpperEdgeGraph() override = default declaration.
* utility/options/{Boolean,File,Integer,Real}Option(.hh) and their
*VectorOption.hh counterparts: empty ~XxxOption() override {}; the
base option classes' virtual destructors are still inherited.
Pull Request №684 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/metric-value-default-members
Merge: b8e64ecdc0338b77c231760f7f9bdcd4d500a240←e91f32a5c7a39c47bdcdb878354f61481e280532
Default MetricValue special members
----------------
Merge commit message:
Fix -Werror=unused-parameter on defaulted MetricValue copy constructor
CI build (gcc with -Werror=unused-parameter) flagged the named parameter
on the defaulted copy constructor as unused. Drop the name; the defaulted
body still copies the source.
Pull Request №691 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/key-containers-rule-of-zero
Merge: b8e64ecdc0338b77c231760f7f9bdcd4d500a240←9ee8c4c268c7bf060c2b014c3c94d2fca90685c5
Apply Rule of Zero to utility/keys container family
----------------
Merge commit message:
Apply Rule of Zero to utility/keys container family
ClassKeyMap, ClassKeyVector, KeyVector, SmallKeyMap, and SmallKeyVector each
held standard-container value-type members only (Vector, IndexMap, plus a
scalar Index in the Small variants). Their user-declared destructors
(empty body or = default), copy constructors, and copy-assignment operators
were all byte-for-byte equivalent to the implicit defaults that the compiler
would synthesize, so they were redundant.
Removing them lets the implicitly defaulted special-member-functions take
over and, as a side effect, restores the implicit move constructor and move
assignment that were previously suppressed by the user-declared copy
operations. The explicit default constructors on SmallKeyMap and
SmallKeyVector are kept because they value-initialize the scalar Index u_,
which an implicit default would leave indeterminate.
Pull Request №690 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/bitset-bitvector-rule-of-zero
Merge: b8e64ecdc0338b77c231760f7f9bdcd4d500a240←7f670472fa0942db38fbaf9eb58d619b79187a4d
Apply Rule of Zero to BitSet and BitVector
----------------
Merge commit message:
Apply Rule of Zero to BitSet and BitVector
Both class templates have a single value-type member (std::set / std::vector)
and no user-declared copy/move/assignment operators, so the explicit empty
destructors did nothing the compiler-generated ones wouldn't. They were also
suppressing the implicit move constructor and move assignment.
Remove the empty `~BitSet() {}` and `~BitVector() {}` declarations so the
classes follow the Rule of Zero and gain implicit move support.
Pull Request №689 RosettaCommons/rosetta/main ← lyskov-ai/rosetta/refactor/disulfide-energy-iterators-rule-of-zero
Merge: b8e64ecdc0338b77c231760f7f9bdcd4d500a240←2da5ceb881f6c2016ec94af5cd47955000095865
Apply Rule of Zero to disulfide energy neighbor iterators
----------------
Merge commit message:
Apply Rule of Zero to disulfide energy neighbor iterators
The disulfide neighbor iterator classes in core/scoring/disulfides/
each had a user-declared destructor (defaulted in the .cc) and an
old-style private undefined copy assignment. The destructor did
nothing the compiler-generated virtual destructor wouldn't do, so
remove both the declaration and the empty `= default` definition.
Modernize the C++03-style copy-assignment prevention to public
`= delete`. The intent is preserved: assignment must go through the
polymorphic operator=(BaseType const &) override, which downcasts and
copies all members; the implicit derived-derived operator= would
bypass that path. A short comment captures this why.
Touched: CentroidDisulfideNeighborIterator/ConstIterator,
DisulfideMatchingNeighborIterator/ConstIterator,
DisulfResNeighbIterator/ConstIterator.