Skip to content

Refactor bmqu::AlignedPrinter to accept vector of strings #895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor bmqu::AlignedPrinter to accept vector of strings #895

wants to merge 4 commits into from

Conversation

kapil0x
Copy link

@kapil0x kapil0x commented Aug 24, 2025 •
edited
Loading

Replace bsl::vector<const char*> with bsl::vectorbsl::string in:

  • Constructor signature and member variable
  • All length calculations (strlen -> .length())
  • All usage examples and documentation
  • All calling code across the codebase

Benefits:

  • Eliminates raw pointer usage for better memory safety
  • Removes manual string length calculations
  • Follows modern C++ best practices
  • Maintains full backward compatibility in functionality

Fixes #564

Issue number of the reported bug or feature request: #

Describe your changes

Refactor bmqu::AlignedPrinter to use bsl::vectorbsl::string instead of bsl::vector<const char*> for improved type safety and modern C++ practices.

Key Changes:

  • Constructor signature: Changed parameter from const bsl::vector<const char*>* to const bsl::vectorbsl::string*
  • String operations: Replaced manual bsl::strlen() calls with .length() method calls
  • Documentation: Updated usage examples and inline documentation to reflect new API
  • Consistency improvements: Changed push_back() to emplace_back() throughout codebase
  • Updates: Fixed all 6 usage sites across different components
  • Fixed JsonPrinter compatibility with AlignedPrinter interface changes as they are both passed as templates in CslRecordPrinter

Testing performed

Unit Testing:

  • Created comprehensive test suite: bmqu_alignedprinter.t.cpp following Bloomberg testing conventions
  • Test coverage includes:
    • Basic functionality with bsl::string vector fields
    • Field alignment with mixed field name lengths
    • Precondition validation for empty fields vector
    • Assertion behavior in debug vs release modes

Additional context

Addresses Issue #564:

This PR directly resolves the "Refactor bmqu::AlignedPrinter to accept vector of strings" issue, implementing the requested modernization of the string handling approach.

Learning from Previous Attempts:

Analyzed failed PR #605 to avoid common pitfalls:

  • Clean integration: No merge conflicts, surgical changes only
  • Proper standards: DCO signoff, proper commit messages
  • Complete solution: Unlike previous attempt, includes comprehensive unit tests

Backward Compatibility:

  • Functional equivalence: Maintains identical output formatting and behavior
  • API migration: Simple drop-in replacement for existing code
  • Zero breaking changes: All existing usage patterns preserved

Code Quality Improvements:

  • First unit tests: AlignedPrinter previously had no dedicated test coverage
  • Documentation updates: Usage examples now reflect best practices
  • Consistency: Standardized on emplace_back() throughout modified files

@kapil0x kapil0x force-pushed the refactor-aligned-printer-564 branch from 7a4041a to 34b70d7 Compare August 24, 2025 22:32
@kapil0x kapil0x marked this pull request as ready for review August 24, 2025 22:47
@kapil0x kapil0x requested a review from a team as a code owner August 24, 2025 22:47
@kapil0x
Copy link
Author

kapil0x commented Aug 24, 2025 •
edited
Loading

@bloomberg/blazingmq-maintainers : Can you please take a look at the PR and approve running workflows on it? The PR is not ready for review yet!

I saw review comments #605 (comment) from another PR that was submitted for the same issue.

This PR's scope is about AlignedPrinter refactoring (issue #564)
The AppIdLengthPair changes would be a separate string modernization effort
Mixing these changes would make the PR less focused and harder to review
Would you like to continue with the AlignedPrinter changes in this PR and handle the AppIdLengthPair refactoring as a separate task?

@kapil0x kapil0x marked this pull request as draft August 24, 2025 22:56
@kapil0x kapil0x force-pushed the refactor-aligned-printer-564 branch 3 times, most recently from ae9a976 to d2fd5a0 Compare August 25, 2025 18:54
@kapil0x kapil0x marked this pull request as ready for review August 25, 2025 20:00
@kapil0x kapil0x mentioned this pull request Aug 28, 2025
Open
5 tasks
@KapilJ22
Apply PR bloomberg#895: Refactor bmqu::AlignedPrinter to accept vecto…
1ce9b0e 
…r of strings

Complete implementation from PR bloomberg#895:
- Updated AlignedPrinter constructor to accept bsl::vector<bsl::string>*
- Updated JsonPrinter to maintain API consistency
- Replaced bsl::strlen() with bsl::string::length() calls
- Added comprehensive unit test suite (bmqu_alignedprinter.t.cpp)
- Updated all usage sites across bmqstoragetool, bmqtool, and mqbs modules
- Optimized performance with emplace_back() instead of push_back()
- Preserved const char* usage for hash/data pointers (not string literals)

Files modified:
- Core classes: bmqu_alignedprinter.h, bmqu_jsonprinter.h
- Test suite: bmqu_alignedprinter.t.cpp (new)
- Tools: m_bmqstoragetool_*, m_bmqtool_storageinspector.cpp
- Storage: mqbs_filestoreprotocolprinter.*

This addresses issue bloomberg#564 by eliminating raw pointer usage for string
literals while improving memory safety and following modern C++ practices.

Signed-off-by: Kapil Jain <[email protected]>
@kapil0x kapil0x force-pushed the refactor-aligned-printer-564 branch from 521d8dd to d40cad8 Compare August 28, 2025 22:36
@KapilJ22
Fix formatting issues
8b94d64 
Apply clang-format to ensure code adheres to project formatting standards.

Signed-off-by: Kapil Jain <[email protected]>
@kapil0x kapil0x force-pushed the refactor-aligned-printer-564 branch from 9502cf9 to 8b94d64 Compare August 28, 2025 22:45
@KapilJ22
Remove temporary patch file
f97164c 
The pr-895.patch file was used temporarily to apply changes and should not be included in the final commit.

Signed-off-by: Kapil Jain <[email protected]>
@kapil0x kapil0x force-pushed the refactor-aligned-printer-564 branch from 9a20961 to f97164c Compare August 29, 2025 23:26
@KapilJ22
Fix GitHub workflow syntax errors
024cc96 
Replace double quotes with single quotes in GitHub Actions if conditions to fix YAML parsing errors.

Signed-off-by: Kapil Jain <[email protected]>
678098
678098 reviewed Sep 3, 2025
View reviewed changes
Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

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

Have some comments

src/groups/bmq/bmqu/bmqu_alignedprinter.h
, d_counter(0)
{
BSLS_ASSERT_SAFE(0 <= d_indent);
BSLS_ASSERT_SAFE(0 < d_fields_p->size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
BSLS_ASSERT_SAFE(0 < d_fields_p->size());
BSLS_ASSERT_SAFE(d_fields_p);
BSLS_ASSERT_SAFE(0 < d_fields_p->size());

There is a small overlook here, we should probably check that the provided fields arg is not NULL before dereferencing it

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp
Comment on lines +54 to +65
bmqu::MemOutStream stream;
bsl::vector<bsl::string> fields;
fields.emplace_back("Queue URI");
fields.emplace_back("QueueKey");
fields.emplace_back("Number of AppIds");

const int indent = 4;
bmqu::AlignedPrinter printer(stream, &fields, indent);

// Test printing various types
bsl::string uri = "bmq://bmq.tutorial.workqueue/sample-queue";
bsl::string queueKey = "sample";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bmqu::MemOutStream stream;
bsl::vector<bsl::string> fields;
fields.emplace_back("Queue URI");
fields.emplace_back("QueueKey");
fields.emplace_back("Number of AppIds");
const int indent = 4;
bmqu::AlignedPrinter printer(stream, &fields, indent);
// Test printing various types
bsl::string uri = "bmq://bmq.tutorial.workqueue/sample-queue";
bsl::string queueKey = "sample";
bslma::Allocator* alloc = bmqtst::TestHelperUtil::allocator();
bmqu::MemOutStream stream(alloc);
bsl::vector<bsl::string> fields(alloc);
fields.emplace_back("Queue URI");
fields.emplace_back("QueueKey");
fields.emplace_back("Number of AppIds");
const int indent = 4;
bmqu::AlignedPrinter printer(stream, &fields, indent);
// Test printing various types
bsl::string uri("bmq://bmq.tutorial.workqueue/sample-queue", alloc);
bsl::string queueKey("sample", alloc);

We have default allocator checks in UTs

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp

printer << uri << queueKey << numAppIds;

bsl::string output = stream.str();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bsl::string output = stream.str();
bsl::string output(stream.str(), alloc);

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp
Comment on lines +98 to +108
bmqu::MemOutStream stream;
bsl::vector<bsl::string> fields;
fields.emplace_back("Short");
fields.emplace_back("Very Long Field Name");
fields.emplace_back("Med");

bmqu::AlignedPrinter printer(stream, &fields, 2);

printer << "value1" << "value2" << "value3";

bsl::string output = stream.str();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bmqu::MemOutStream stream;
bsl::vector<bsl::string> fields;
fields.emplace_back("Short");
fields.emplace_back("Very Long Field Name");
fields.emplace_back("Med");
bmqu::AlignedPrinter printer(stream, &fields, 2);
printer << "value1" << "value2" << "value3";
bsl::string output = stream.str();
bslma::Allocator* alloc = bmqtst::TestHelperUtil::allocator();
bmqu::MemOutStream stream(alloc);
bsl::vector<bsl::string> fields(alloc);
fields.emplace_back("Short");
fields.emplace_back("Very Long Field Name");
fields.emplace_back("Med");
bmqu::AlignedPrinter printer(stream, &fields, 2);
printer << "value1" << "value2" << "value3";
bsl::string output(stream.str(), alloc);

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp
}

TEST_EPILOG(bmqtst::TestHelper::e_DEFAULT);
} No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp
#ifdef BDE_BUILD_TARGET_SAFE_2
// In safe mode, this should assert
BMQTST_ASSERT_SAFE_FAIL(bmqu::AlignedPrinter(stream, &fields, 4));
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this test is not necessary. It checks a basic precondition in the code, and this precondition is just 1 line and much simpler than the test itself.

src/groups/bmq/bmqu/bmqu_alignedprinter.t.cpp
} break;
}

TEST_EPILOG(bmqtst::TestHelper::e_DEFAULT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TEST_EPILOG(bmqtst::TestHelper::e_DEFAULT);
TEST_EPILOG(bmqtst::TestHelper::e_CHECK_DEF_GBL_ALLOC);

@678098 678098 assigned 678098 and kapil0x and unassigned 678098 Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@678098 678098 678098 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@kapil0x kapil0x

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor bmqu::AlignedPrinter to accept vector of strings

3 participants

@kapil0x @678098 @KapilJ22