Skip to content

Conversation

@Spartan322
Copy link
Member

@Spartan322 Spartan322 commented Oct 14, 2025

wvpm
wvpm previously requested changes Oct 14, 2025
Copy link
Contributor

@wvpm wvpm left a comment

Choose a reason for hiding this comment

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

Don't use macros to log 1 message and return.
Require context for log messages.

@Spartan322
Copy link
Member Author

Logger::error already gives context, we don't need to add anything to that.

@Spartan322 Spartan322 dismissed wvpm’s stale review October 15, 2025 02:09

Your review does not understand how these macros work.

wvpm
wvpm previously requested changes Oct 15, 2025
Copy link
Contributor

@wvpm wvpm left a comment

Choose a reason for hiding this comment

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

Don't use macros to log 1 message and return.
Require context for log messages.
"Index " _OV_STR(m_index) " = ", m_index

@Spartan322 Spartan322 dismissed wvpm’s stale review October 15, 2025 07:47

You review is still wrong.

@Spartan322 Spartan322 force-pushed the add/more-error-macros branch from 9f40b50 to 4dfb83a Compare October 15, 2025 07:55
Hop311
Hop311 previously approved these changes Oct 17, 2025
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

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

Some stuff could be slightly cleaned up but overall I welcome this, I can help switch stuff to using it once it's merged (especially places we could benefit from unlikely)

Fix error macro names in comments
@Hop311 Hop311 merged commit fc7d49a into master Oct 18, 2025
16 checks passed
@Hop311 Hop311 deleted the add/more-error-macros branch October 18, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request topic:core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants