Skip to content

Adds a way to control chunkspan print formating#1106

Open
PaulGannay wants to merge 6 commits intoCExA-project:mainfrom
PaulGannay:print_option
Open

Adds a way to control chunkspan print formating#1106
PaulGannay wants to merge 6 commits intoCExA-project:mainfrom
PaulGannay:print_option

Conversation

@PaulGannay
Copy link
Copy Markdown
Contributor

@PaulGannay PaulGannay commented Apr 2, 2026

Should solve: #1059

@PaulGannay PaulGannay requested a review from tpadioleau as a code owner April 2, 2026 14:42
@PaulGannay
Copy link
Copy Markdown
Contributor Author

PaulGannay commented Apr 2, 2026

CI reports:

Run if grep -R "#include <iostream>" $(git ls-files 'src/*.[ch]pp'); then exit 1; fi
  if grep -R "#include <iostream>" $(git ls-files 'src/*.[ch]pp'); then exit 1; fi
  shell: /usr/bin/bash -e {0}
src/ddc/print.cpp:#include <iostream>
Error: Process completed with exit code 1.

iostream is included but in a .cpp file which won't be visible to the end user, this check looks like it was written when the library was header only?

Comment on lines 27 to 32
std::unique_ptr<char, decltype(std::free)*> const
demangled_name(abi::__cxa_demangle(mangled_name, nullptr, nullptr, &status), std::free);
if (status != 0) {
os << "Error demangling dimension name: " << status;
return;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should maybe be changed to output th error to cerr?
And only print Unknown type to os?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • I agree about outputing to cerr, I wonder how we can test that ? Should we pass a second std::ostream ?
  • What about printing the given mangled_name ?

Copy link
Copy Markdown
Contributor Author

@PaulGannay PaulGannay Apr 3, 2026

Choose a reason for hiding this comment

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

What about printing the given mangled_name ?

You are right, this is a better solution

I agree about outputing to cerr, I wonder how we can test that ? Should we pass a second std::ostream ?

I don't think we need to test the output to cerr, testing that we get the expected mangled type back should be enough?

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

❌ Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.87%. Comparing base (24f77da) to head (203f871).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/ddc/print.hpp 95.12% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1106      +/-   ##
==========================================
- Coverage   94.90%   94.87%   -0.03%     
==========================================
  Files          64       64              
  Lines        2844     2871      +27     
  Branches      893      909      +16     
==========================================
+ Hits         2699     2724      +25     
  Misses         95       95              
- Partials       50       52       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tpadioleau
Copy link
Copy Markdown
Member

CI reports:

Run if grep -R "#include <iostream>" $(git ls-files 'src/*.[ch]pp'); then exit 1; fi
  if grep -R "#include <iostream>" $(git ls-files 'src/*.[ch]pp'); then exit 1; fi
  shell: /usr/bin/bash -e {0}
src/ddc/print.cpp:#include <iostream>
Error: Process completed with exit code 1.

iostream is included but in a .cpp file which won't be visible to the end user, this check looks like it was written when the library was header only?

I agree, the grep applies to .cpp source files but I it seems too restrictive. Let me update that rule.

Copy link
Copy Markdown
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

Overall good for me. What do you think of making getInstance a free function and making the constructor a public function ? That would allow to make ChunkPrinter independent of the singleton.

Comment on lines 27 to 32
std::unique_ptr<char, decltype(std::free)*> const
demangled_name(abi::__cxa_demangle(mangled_name, nullptr, nullptr, &status), std::free);
if (status != 0) {
os << "Error demangling dimension name: " << status;
return;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • I agree about outputing to cerr, I wonder how we can test that ? Should we pass a second std::ostream ?
  • What about printing the given mangled_name ?

{
// Ensure that m_edgeitems < (m_threshold / 2) stays true.
ddc::detail::ChunkPrinter& printer = ddc::detail::ChunkPrinter::getInstance();
printer.m_global_lock.lock();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I remember correctly there is a RAII version of locking in the standard, could be useful in case an exception happens before unlock ?



ddc::detail::ChunkPrinter& printer = ddc::detail::ChunkPrinter::getInstance();
printer.m_global_lock.lock();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should locking be hidden in the member functions ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is better to lock in print_type_info to ensure that the metadata and the content of the chunkspan get printed together.

PaulGannay and others added 3 commits April 3, 2026 10:34
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
Signed-off-by: Paul Gannay <paul.gannay@cea.fr>
Signed-off-by: Paul Gannay <paul.gannay@cea.fr>
@PaulGannay PaulGannay force-pushed the print_option branch 4 times, most recently from 1d7c026 to 1224d78 Compare April 3, 2026 13:55
 - full_print function to always print full array ignoring options
 - use struct to set the options in order to use designated initializers
 - use scoped_lock instead of lock_guard to follow linter recommendation

Signed-off-by: Paul Gannay <paul.gannay@cea.fr>
Signed-off-by: Paul Gannay <paul.gannay@cea.fr>
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.

2 participants