Skip to content

Conversation

@WZhuo
Copy link
Contributor

@WZhuo WZhuo commented Dec 3, 2025

No description provided.

@WZhuo WZhuo force-pushed the memory_catalog branch 4 times, most recently from 55d3900 to 67a5362 Compare December 5, 2025 07:41
Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

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

LGTM

const std::vector<std::unique_ptr<TableRequirement>>& requirements,
const std::vector<std::unique_ptr<TableUpdate>>& updates) {
return NotImplemented("update table");
std::unique_lock lock(mutex_);
Copy link
Contributor

@HuaHuaY HuaHuaY Dec 5, 2025

Choose a reason for hiding this comment

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

Suggested change
std::unique_lock lock(mutex_);
std::lock_guard guard(mutex_);

Use std::lock_guard instead of std::unique_lock if we don't need to move the guard.

Copy link
Contributor Author

@WZhuo WZhuo Dec 8, 2025

Choose a reason for hiding this comment

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

I think it can be optimized with ReadWriteLock later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we may be able to replace the lock type with std::shared_lock. If you don't do that in this PR, I hope that you can replace with std::lock_guard first. Because we have merged a PR #398 which replaced the other unique_lock in this file.

@wgtmac
Copy link
Member

wgtmac commented Dec 9, 2025

This PR seems have carried other unrelated changes. Could you fix it?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants