Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

A few cleanup/refactoring commits. #76

Merged
merged 3 commits into from
May 9, 2017
Merged

Conversation

frewsxcv
Copy link
Contributor

No description provided.

@wycats
Copy link
Contributor

wycats commented Apr 27, 2017

@frewsxcv for the cases where we're making c-strings, does it make sense to use our cstr! macro that we use in other places?

@frewsxcv
Copy link
Contributor Author

frewsxcv commented Apr 29, 2017

The cstr! macro only works when we're converting a string literal to a CStr, which is not the case for these lines. If you wanted to avoid doing this CString allocation here, you'd need to make the arguments to these functions/methods a &CStr instead of a &str, like this. Thoughts?

@wagenet wagenet requested a review from wycats April 29, 2017 19:48
@wycats
Copy link
Contributor

wycats commented May 1, 2017

@frewsxcv I think that's acceptable. We don't intend the APIs you're changing to be ergonomic public APIs, but rather implementation details of the macro.

frewsxcv added a commit to frewsxcv/helix that referenced this pull request May 3, 2017
frewsxcv added a commit to frewsxcv/helix that referenced this pull request May 3, 2017
@frewsxcv
Copy link
Contributor Author

frewsxcv commented May 3, 2017

Opened a PR: #88

Will rebase this once that lands

@frewsxcv
Copy link
Contributor Author

frewsxcv commented May 5, 2017

rebased

@wagenet wagenet merged commit 4117210 into tildeio:master May 9, 2017
@wagenet
Copy link
Collaborator

wagenet commented May 9, 2017

Thanks!

@frewsxcv frewsxcv deleted the cleanup branch May 10, 2017 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants