Skip to content
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.

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

Add stream-style logging macros #786

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dentiny
Copy link

@dentiny dentiny commented Dec 10, 2024

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

// 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
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
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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds a requested feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants