Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Conversation

@igorpeshansky
Copy link
Contributor

No description provided.

Copy link
Contributor

@bmoyles0117 bmoyles0117 left a comment

Choose a reason for hiding this comment

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

LGTM!

std::lock_guard<std::mutex> lock(store_.resource_mu_);
const auto result = store_.resource_map_.find(id);
if (result == store_.resource_map_.end()) {
if (result != store_.resource_map_.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The overall change looks good.

Not sure about this commit though - why the need to reverse the if/else?
Positive comparisons are easier to read (harder to misread) anyway.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Mar 27, 2018

Choose a reason for hiding this comment

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

This was just to make the change from if/else to try/catch in the next commit easier to see (otherwise it might get lost in the delete/add of the move).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. I was reading the commits the wrong way around.

Copy link
Contributor

@supriyagarg supriyagarg left a comment

Choose a reason for hiding this comment

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

LGTM

@igorpeshansky igorpeshansky merged commit ead745e into master Mar 27, 2018
@igorpeshansky igorpeshansky deleted the igorp-metadata-store-lookup branch March 27, 2018 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants