Skip to content

String buffer fix#239

Open
Sam Clarke-Green (t00sa) wants to merge 7 commits intoMetOffice:mainfrom
t00sa:string-buffer-fix
Open

String buffer fix#239
Sam Clarke-Green (t00sa) wants to merge 7 commits intoMetOffice:mainfrom
t00sa:string-buffer-fix

Conversation

@t00sa
Copy link
Collaborator

Description

Fix a bug which prevented the string buffer CMake parameter from setting the PROF_STRING_BUFFER_LENGTH pre-processor macro correctly. Also improve the error message the results if the region name length is too long.

Linked issues

Closes #226

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • New tests have been added
  • Tests have been modified to accommodate this change

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes, for both debug and optimised builds

@github-actions github-actions bot added the cla-required The CLA has not yet been signed by the author of this PR - added by GA label Mar 26, 2026
@github-actions github-actions bot added cla-signed The CLA has been signed as part of this PR - added by GA and removed cla-required The CLA has not yet been signed by the author of this PR - added by GA labels Mar 26, 2026
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested that this works correctly allowing the correct length of calliper names when built and installed.

Just a couple of small changes to make the error message more meaning full to users.

if (region_name.length() + num_extra_bytes > new_chars.size()) {
error_handler("Internal error: character buffer exhausted.", EXIT_FAILURE);
std::stringstream message;
message << "Internal error: region name too long ("

Choose a reason for hiding this comment

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

Just to make this obvious it comes from Vernier, I intend to refactor logging to be better but we might as well make this message obvious now.

Suggested change
message << "Internal error: region name too long ("
message << "Vernier internal error: region name too long ("


EXPECT_EXIT(meto::vernier.start(label), testing::ExitedWithCode(1),
"Internal error: character buffer exhausted.");
HasSubstr("Internal error: region name too long"));

Choose a reason for hiding this comment

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

Suggested change
HasSubstr("Internal error: region name too long"));
HasSubstr("Vernier internal error: region name too long"));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The CLA has been signed as part of this PR - added by GA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Custom Issue]: Character buffer error message improvement

2 participants