Skip to content

C++: Model std::vector emplace and emplace_back()#4589

Merged
geoffw0 merged 3 commits into
github:mainfrom
criemen:model-vector-emplace
Nov 4, 2020
Merged

C++: Model std::vector emplace and emplace_back()#4589
geoffw0 merged 3 commits into
github:mainfrom
criemen:model-vector-emplace

Conversation

@criemen

@criemen criemen commented Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Fixes https://github.com/github/codeql-c-analysis-team/issues/159.
I recommend commit-by-commit review for the test changes, as the addition to stl.h lead to some churn.

@criemen criemen requested a review from a team as a code owner November 2, 2020 10:55
@github-actions github-actions Bot added the C++ label Nov 2, 2020
@geoffw0

geoffw0 commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

LGTM.

Do you think it will be valuable to extend this to std::array, std::deque, std::list and std::forward_list at some point (to the extent that they have similar emplace methods) - or is vector::emplace the vast majority of usage anyway?

@dbartol in our meeting last week you asked us to try and avoid merging changes to taint tracking while you finished getting a big PR merged. Is that done, and if not is this change likely to conflict?

@criemen

criemen commented Nov 3, 2020

Copy link
Copy Markdown
Collaborator Author

I haven't looked closer at the other classes that we don't have support for, but at lest for std::set, std::vector and std::map emplace has slightly different semantics (arguments, return value), so our implementation does not generalize.
The question about usage could be best answered by a query run across LGTM I guess?

Let's wait with merging this until #4432 lands, as we do touch the same test files.

@geoffw0

geoffw0 commented Nov 3, 2020

Copy link
Copy Markdown
Contributor

It looks like there are a few uses of the others, particularly list and deque, but not as many as for vector. There are also a lot of uses on a class _Hashtable, which appears to be a commonly occurring implementation class(?). So modelling these is not as important as the vector and map work.

@criemen criemen force-pushed the model-vector-emplace branch from f610831 to e7e5754 Compare November 4, 2020 15:21

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#4432 is merged, so I will merge this as soon as I'm able now.

@geoffw0 geoffw0 merged commit 48628fa into github:main Nov 4, 2020
@criemen criemen deleted the model-vector-emplace branch November 4, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants