Skip to content

Rename vector to list #1016

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

Merged
merged 3 commits into from
Aug 10, 2020
Merged

Rename vector to list #1016

merged 3 commits into from
Aug 10, 2020

Conversation

mavam
Copy link
Member

@mavam mavam commented Aug 10, 2020

No description provided.

@mavam mavam added the refactoring Restructuring of existing code label Aug 10, 2020
@mavam mavam requested a review from dominiklohmann August 10, 2020 14:57
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Just some minor things.

Also, please add a separate changelog entry for this PR.

@@ -45,7 +45,7 @@ struct fixture {
}

type t;
vector r;
std::vector<data> r;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<data> r;
list r;

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment below for why going with std::vector<data>.

@@ -55,7 +55,7 @@ FIXTURE_SCOPE(event_tests, fixture)

TEST(basics) {
CHECK_EQUAL(e.type().name(), "foo");
REQUIRE(caf::holds_alternative<vector>(e.data()));
REQUIRE(caf::holds_alternative<std::vector<data>>(e.data()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
REQUIRE(caf::holds_alternative<std::vector<data>>(e.data()));
REQUIRE(caf::holds_alternative<list>(e.data()));

Copy link
Member Author

Choose a reason for hiding this comment

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

Here, list is the wrong way to think about this. An event is a sequence of data, not an instance of list. This is why I changed it to std::vector<data>. It'll change with the record change in data as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is just unnecessarily confusing. A list now is exactly a sequence of data, because that is quite literally its definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it's a value for a single field. Here, we are modeling a sequence of values, each of which represent a single field.

Copy link
Member

Choose a reason for hiding this comment

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

I still disagree with you, but not not enough to argue about this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, it'll change soon anyway once record is there.

@mavam mavam requested a review from dominiklohmann August 10, 2020 15:18
@mavam mavam merged commit 439b835 into master Aug 10, 2020
@mavam mavam deleted the story/ch18751 branch August 10, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Restructuring of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants