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

Changed ArgMap to be backed by a vector instead of a map. #262

Merged
merged 1 commit into from
Jan 12, 2016

Conversation

mwinterb
Copy link
Contributor

The main reason for this is to avoid a dynamic memory allocation in every format() call with Visual Studio if there are no named arguments. Addresses #261.

The main reason for this is to avoid a dynamic memory allocation in every format() call with Visual Studio if there are no named arguments.
@vitaut
Copy link
Contributor

vitaut commented Jan 11, 2016

Thanks for the PR. One obvious improvement for large number of named arguments is to sort the vector and use binary search, but it can be done separately.

@jamboree, do you have any comments regarding this?

@jamboree
Copy link
Contributor

I'd have had used boost::flat_map if I could. That said, the source code is usually written by human, I think in practice there won't be too many arguments, so linear search is probably enough.

// the list is unsorted, so just return the first matching name.
for (; it != map_.end(); ++it) {
if (it->first == name)
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer return early here and prevent the unnecessary check in the end.

@mwinterb
Copy link
Contributor Author

My general thought on sorted or not matches @jamboree's, but if sorting is desired I'd rather get that out of the way now rather than need to break binary compat again later. Edit: Actually, I'm assuming that binary compat would be broken since there are several indirect callers of find in the header, but I haven't actually analyzed if those where they would be inlined in the non-header only mode.

Also the handling of duplicate names is basically the same as it was before (ignoring them), but it doesn't match what Pythons str.format does. Should duplicated named arguments throw an exception or should they remain ignored?

@jamboree
Copy link
Contributor

@mwinterb, if you decide to check duplicate names, I'd suggest using assertions since it's definitely a programmer mistake that should be found in testing.

@vitaut
Copy link
Contributor

vitaut commented Jan 12, 2016

Another option is to use some kind of a hash table, but unless someone is really interested in investing time in benchmarking different options, I wouldn't worry about it and just used linear search.

I agree with @jamboree that the check for duplicate names, if introduced, better be done as an assertion.

vitaut added a commit that referenced this pull request Jan 12, 2016
Changed ArgMap to be backed by a vector instead of a map.
@vitaut vitaut merged commit 50f14f2 into fmtlib:master Jan 12, 2016
@vitaut
Copy link
Contributor

vitaut commented Jan 12, 2016

Merged with a minor change in 06f3abe to return early from ArgMap::find as suggested by @jamboree. Any improvements are welcome as separate PRs. Thanks!

@mwinterb mwinterb deleted the argmap_vector branch January 12, 2016 22:44
@mwinterb
Copy link
Contributor Author

Thanks @vitaut!

And on throw vs assert, I was thinking that it'd be like if the format string was wrong, but since the format strings may come from an external source and argument names won't, assert is the right choice.

If the need for catching duplicate argument names comes up at some point I'll make a PR with that.

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