Skip to content

List and pair - #8

Merged
satac2 merged 8 commits into
masterfrom
list_and_pair
Jun 28, 2020
Merged

satac2 merged 8 commits into
masterfrom
list_and_pair

Conversation

@satac2

@satac2 satac2 commented Jun 24, 2020

Copy link
Copy Markdown
Owner

nostd implementations of list and pair, they are by no means a perfect copy of the std implementation. However, I feel like I don't want to put too much more time into it, until we know if we are going to move forward with this approach.

@satac2
satac2 requested review from 0x4b and adityashirodkar June 24, 2020 17:05
Comment thread api/include/opentelemetry/nostd/list.h
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
int counter = 1;
for (auto iter = std::begin(int_list); iter != std::end(int_list); ++iter)
{
EXPECT_EQ(*iter, counter);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There might be a reason it won't work for nostd::list, but there's a ElementsAre gtest matcher that you could try using like

EXPECT_THAT(int_list, ElementsAre(1,2,3,4,5,6,7,8));

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I am having some trouble finding the header that contains ElementsAre

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It should be in gmock/gmock.h

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I have tried gmock/gmock.h and am getting "'ElementsAre' was not declared in this scope"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably you're missing the namespace? testing::ElementsAre

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

It looks like in order to use ElementsAre, I would also need to implement a const iterator for nostd::list. Do you think that would be valuable?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wouldn't really recommend extending the nostd::list interface specifically for testing, but a const iterator seems like it would be useful generally. Do it if you think it will be quick and you have some spare time.

Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated
int counter = 1;
for (auto iter = std::begin(int_list); iter != std::end(int_list); ++iter)
{
EXPECT_EQ(*iter, counter);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread api/include/opentelemetry/nostd/list.h Outdated
Comment thread api/include/opentelemetry/nostd/pair.h Outdated
pair<int, int> pair_1 = pair<int, int>(val_1, val_2);
pair<int, int> pair_2 = pair<int, int>(val_1, val_2);

EXPECT_EQ(pair_1 == pair_2, true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just FYI: Since pair::operator== is defined I think you can do
EXPECT_EQ(pair_1, pair_2)
and
EXPECT_NE(pair_1, pair_2)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

When I try, it spits out a number of errors, does that imply that the == operator is not implemented properly?

Comment thread api/test/nostd/list_test.cc Outdated
Comment thread api/test/nostd/list_test.cc Outdated

@adityashirodkar adityashirodkar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall PR looks great. I think the biggest suggestion I would have is being consistent with formatting your code.

For example various ways functions are formatted in your files.

void fun()
{

// do something

}

vs

void fun()
{
// do something
}

vs

void fun() {
// do something
}

I recommend taking a look at https://google.github.io/styleguide/cppguide.html#Formatting

Comment thread api/test/nostd/list_test.cc Outdated
Comment thread api/test/nostd/list_test.cc Outdated
Comment thread api/test/nostd/list_test.cc Outdated
Comment thread api/test/nostd/list_test.cc Outdated
Comment thread api/test/nostd/pair_test.cc
Comment thread api/include/opentelemetry/nostd/pair.h Outdated
Comment thread api/include/opentelemetry/nostd/list.h Outdated

@adityashirodkar adityashirodkar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@satac2
satac2 merged commit f3bba41 into master Jun 28, 2020
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.

3 participants