Skip to content

Add stream-style logging macros #786

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
Merged
merged 5 commits into from
Aug 26, 2025
Merged

Add stream-style logging macros #786

merged 5 commits into from
Aug 26, 2025

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 10, 2024 •
edited
Loading

Problem statement:

  • Existing logging and assertion is kind of awkward, for example, it doesn't accept stream-style operation, meanwhile it uses string as the macro parameter, which means we have to concatenate the message beforehand.
    • (minor) It might hurts performance, I see a lot of the message construction is made by operator+, which could create temporary strings
    • (major) it reduces logging util usability

In this PR, I propose to add a new macro, which combines both assertion and logging with stream style operation.

How I tested:
https://onlinegdb.com/PZiB5VT2A

dentiny
dentiny commented Dec 10, 2024
View reviewed changes
src/include/common/macros.h Outdated
// A macro which checks `expr` value and performs assert.
// Different from `BUSTUB_ASSERT`, it takes stream-style parameters.
#define BUSTUB_ASSERT_AND_LOG(expr) \
if (bool val = (expr); !val) LogFatalStream { \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think necessary, I could add a Unlikely function, which improves branch prediction, since I suspect all assertions sits on critical path.

@apavlo
Copy link
Member

apavlo commented Dec 10, 2024

@dentiny Thanks for sending this. We will take a look at it. It might make more sense to use a third-party header-only logger.

@apavlo apavlo added the feature Adds a requested feature. label Dec 10, 2024
@dentiny
Copy link
Contributor Author

dentiny commented Dec 10, 2024

Thanks @apavlo for taking a look! It would be nice to support CHECK_EQ-like syntax, which

  • Allows comparison logic
  • Prints out value for lhs and rhs if comparison logic fails
  • Code location (i.e. filename and line number) is included in the error message

For this PR, I mostly target at code simplicity w/o extra deps :)

@dentiny dentiny force-pushed the hjiang/add-more-logging-macros branch from fd83a95 to 34d5495 Compare December 11, 2024 12:08
@dentiny
Copy link
Contributor Author

dentiny commented Dec 15, 2024

Curious if there're any updates on this PR? 👀

@apavlo
Copy link
Member

apavlo commented Dec 16, 2024

@dentiny End of semester crunch. We will look at this soon.

connortsui20
connortsui20 approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@connortsui20 connortsui20 left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Sorry for not reviewing this last semester! I guess this will be up to the future TAs to actually merge but LGTM

@connortsui20
Copy link
Member

If you can fix the CI then I think this is fine to merge!

@dentiny
Copy link
Contributor Author

dentiny commented May 17, 2025

If you can fix the CI then I think this is fine to merge!

Thank you @connortsui20 for the review! I think the code is benign so I simply suppress the linter warnings.

@songwdfu songwdfu merged commit 4e1053d into cmu-db:master Aug 26, 2025
songwdfu added a commit that referenced this pull request Aug 26, 2025
@songwdfu
Revert "Add stream-style logging macros (#786)"
b4ec067 
This reverts commit 4e1053d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connortsui20 connortsui20 connortsui20 approved these changes

Assignees
No one assigned
Labels
feature Adds a requested feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dentiny @apavlo @connortsui20 @songwdfu