clang-tidy: resolve cppcoreguidelines-owning-memory#551
Conversation
6741ab7 to
aad1e77
Compare
There was a problem hiding this comment.
Pull request overview
This PR focuses on addressing clang-tidy’s cppcoreguidelines-owning-memory guidance by making ownership intent explicit and reducing raw new/delete usage across FORM ROOT storage, FORM tests, and the Python plugin bindings.
Changes:
- Replaces ad-hoc raw-pointer ownership patterns with clearer ownership annotations (
gsl::owner) and RAII (std::unique_ptr) in storage/test code paths. - Introduces a
std::unique_ptr<TTree, Deleter>pattern for ROOTTTreelifetime handling in the write container. - Wires Microsoft.GSL into the relevant targets (FORM ROOT tests, ROOT storage, and Python plugin) to support
gsl::ownerusage.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/form/test_utils.hpp |
Removes an unnecessary new PROD() in doRead() and reads into a raw pointer before wrapping in unique_ptr. |
test/form/reader.cpp |
Annotates returned pointers as gsl::owner<...*> and links against GSL for ownership annotation support. |
test/form/CMakeLists.txt |
Links Microsoft.GSL::GSL to the ReadVector test. |
plugins/python/src/modulewrap.cpp |
Adds gsl::owner annotations for heap-allocated callback/provider functors (but allocations are still leaked). |
plugins/python/CMakeLists.txt |
Links Microsoft.GSL::GSL into pymodule. |
form/storage/storage_reader.cpp |
Replaces manual delete of read strings with std::unique_ptr to enforce RAII cleanup in getIndex(). |
form/storage/CMakeLists.txt |
Changes storage link visibility to PUBLIC for core and root_storage. |
form/root_storage/root_ttree_write_container.hpp |
Switches m_tree to std::unique_ptr<TTree, TTreeDeleter> and documents ownership intent. |
form/root_storage/root_ttree_write_container.cpp |
Implements TTreeDeleter and uses unique_ptr::reset() for TTree acquisition/creation. |
form/root_storage/root_tbranch_read_container.cpp |
Avoids explicit allocations before SetBranchAddress() by passing a null pointer-to-pointer buffer. |
form/root_storage/demangle_name.cpp |
Marks the demangled buffer as gsl::owner<char*> while still freeing with std::free(). |
form/root_storage/CMakeLists.txt |
Links Microsoft.GSL::GSL (PRIVATE) into root_storage. |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #551 +/- ##
==========================================
+ Coverage 85.29% 85.39% +0.09%
==========================================
Files 145 145
Lines 3679 3710 +31
Branches 632 646 +14
==========================================
+ Hits 3138 3168 +30
- Misses 324 329 +5
+ Partials 217 213 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
595cc8b to
e943cc3
Compare
e943cc3 to
6ba94eb
Compare
6ba94eb to
d62d911
Compare
ffce55e to
0855059
Compare
0855059 to
daf7a96
Compare
daf7a96 to
ee67040
Compare
pcanal
left a comment
There was a problem hiding this comment.
Looks good to me. If https://github.com/Framework-R-D/phlex/pull/551/changes#r3157633667 is not resolved in this PR, there ought to be an issue open to keep track of its resolution.
wwuoneway
left a comment
There was a problem hiding this comment.
LGTM. https://github.com/Framework-R-D/phlex/pull/551/changes#r3157633667 would need a separate issue track.
If non-ROOT allocation of memory is actually required, then the final parameter to SetBranchAddress() should have been `false` (and we should have sent `branchBuffer` instead of `&branchBuffer`). Otherwise, the memory is allocated by ROOT anyway and memory may be leaked—or worse, deallocated possibly inappropriately with `delete [] char`.
Be absolutely clear about the fact that we are managing the TTree's memory ourselves to ensure that it is deleted at the right time, and _after_ we ensure that pending objects have been written to the tree.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Interface library `form_test_utils` encapsulating header-specific dependency `ROOT::Core` - Explicit dependence on `root_storage` instead of accidental via `storage` - Improve coverage provided by `form_storage_test.cpp`
ee67040 to
dead99e
Compare
test/form/reader.cppnew()indoRead()getIndex()while avoiding rawdeletePUBLICnew()operationsunique_ptrwith a custom deleter for clarity and consistency