Skip to content
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

Rename RemoveIgnored -> RemoveMembers #249

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

ajor
Copy link
Contributor

@ajor ajor commented Jul 19, 2023

Also reshuffle CodeGen's passes to fix an alignment bug with removed members.

Change RemoveMembers to actually remove members instead of replacing them with padding. AddPadding must be run afterwards to fill in the gaps.

Also reshuffle CodeGen's passes to fix an alignment bug with removed
members.

Change RemoveMembers to actually remove members instead of replacing
them with padding. AddPadding must be run afterwards to fill in the
gaps.
@ajor ajor marked this pull request as ready for review July 25, 2023 13:26
Member{paddingArray, AddPadding::MemberPrefix, c.members[i].bitOffset};
}
std::erase_if(c.members, [this, &c](Member member) {
return ignoreMember(c.name(), member.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given c.name() returns a copy, can you gather it before and capture it by reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No more copy: #264

}

bool RemoveIgnored::ignoreMember(const std::string& typeName,
bool RemoveMembers::ignoreMember(const std::string& typeName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated but nearby, why is this not checking a set? It seems better to implement a hash function for std::pair<std::string, std::string> than loop over a vector of pairs. Even a map of ContainerName -> Set would be a lot better.

Copy link
Contributor Author

@ajor ajor Jul 26, 2023

Choose a reason for hiding this comment

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

The real reason: It's sharing the data structure with CodeGen v1 and I didn't want to change too much.

But in terms of efficiency, I think the winner of vectors vs maps/sets would come down to how many elements are typically being stored. For a small number of elements a vector would likely win due to the lack of hashing and being more cache friendly.

@ajor ajor merged commit 2d1cc92 into facebookexperimental:main Jul 26, 2023
3 checks passed
@ajor ajor deleted the rename-removed-ignored branch July 26, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants