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

NixPath -> NixString and Error -> Errno #230

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Jan 5, 2016

Major API change, it passes on the responsibility of dealing with CString conversions to the caller. The error type is now just Errno itself.

Closes #221

cc @kamalmarhubi

@kamalmarhubi
Copy link
Member

Is the idea to drop NixString once your impl AsRef<CStr> for CString lands?

@kamalmarhubi
Copy link
Member

Did @carllerche ever comment on #221?

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 5, 2016

Is the idea to drop NixString once your impl AsRef for CString lands?

That's correct, and why I named the method as_ref(). I should add a comment describing that. It will land in 1.7.0. The whole NixPath thing is still waiting on comments from @carllerche, but I needed to make these changes for my own project so I went ahead anyway >.>

@arcnmx arcnmx force-pushed the nixstring-errno branch 5 times, most recently from dc64919 to 780ae21 Compare January 5, 2016 22:10
}
}
}

pub trait ErrnoSentinel: Sized {
Copy link
Member

Choose a reason for hiding this comment

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

A bit of doc for this would be good. After reading I figured out it's how to determine if an error occurred or not from the return value.

@carllerche
Copy link
Contributor

Sorry for the delay! I'm commenting on #221

@arcnmx arcnmx force-pushed the nixstring-errno branch 2 times, most recently from 84c7fbb to 593ede4 Compare January 20, 2016 19:38
@kamalmarhubi
Copy link
Member

Doing some PR triage. This one's going to hang around a bit longer pending #221, I think!

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 25, 2016

Hmmm, now this PR fails because ToOwned wasn't implemented for CStr until rust 1.3.0... Awkward :(

Might have to use a build script to determine version features? I wonder if that would help for also replacing NixString with AsRef<CStr> for 1.7.0+...

@kamalmarhubi
Copy link
Member

Hmmm, now this PR fails because ToOwned wasn't implemented for CStr until rust 1.3.0... Awkward :(

Well #238 hasn't been closed yet... we could discuss a bit in there? There's also some discussion on the users discourse instance which I'll link to on the other issue.

Might have to use a build script to determine version features? I wonder if that would help for also replacing NixString with AsRef for 1.7.0+...

Not sure what you mean there?

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 25, 2016

Not sure what you mean there?

We could add some conditional compilation magic into nix_string.rs:

  1. If the compiler is older than 1.3.0, don't impl NixString on Cow<CStr>
  2. If the compiler is 1.7.0 or newer, don't use NixString at all. Simply define it as pub type NixString = AsRef<CStr>;

This is what the rustc_version crate is for.

@kamalmarhubi
Copy link
Member

I was thinking about how to get further along with what this PR is trying to do. Let's break it down into a few stages:

  • make a PR introducing Errno as a new type, along with its impls, adding an impl From<NixError> for Errno and an impl From<Errno> for NixError for interoperability
  • make a PR introducing NixString along with its impls
  • then we can do a migration over a series of PRs, say one per module
  • remove the old NixError and NixPath when that's all done

I think we can get more people to weigh in on the changes if the PRs are smaller. We can also have more focused discussion of two individual changes.

I also think it'll help because right now pretty much every merged PR will cause conflicts with this one, making it hard to land.

@kamalmarhubi
Copy link
Member

I'm happy to help break it up if you like, just let me know!

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 26, 2016

maybe use the feature name so std_has_cstr_borrow:

mm sorry was just testing on CI. It's probably something that should go in its own PR anyway.

make a PR introducing Errno as a new type, along with its impls, adding an impl From for Errno and an impl From for NixError for interoperability

Errno is already a type, this PR doesn't introduce it. It only removes the Error type because it existed to differentiate errors between Errno and InvalidPath, the latter of which no longer exists due to NixString.

However, we can land Errno::result() first, just as a cleanup that moves the whole repo to the new from_ffi.

  • make a PR introducing NixString along with its impls
  • then we can do a migration over a series of PRs, say one per module
  • remove the old NixError and NixPath when that's all done

This is a strange workflow to me since the intermediate state of the repo would be useless. I guess if we want to break it up I'd do it more like...

  • Whitespace fixes
  • Errno:result()
  • NixString and impls
  • Migrate all methods to NixString.
  • Remove NixError

The last three seem like a single PR to me, breaking it up by module doesn't seem particularly productive as it's just a search-replace NixPath -> NixString. The feedback on that change won't be limited to any particular function or module, it's a fundamental change that raises the question: do we want to remove the stack allocation, copy, and path length check (thus delegating CString conversions to the caller) or not? Landing NixString impls before finalizing that discussion is premature... but once the course of action is chosen there's nothing left to discuss!

@arcnmx arcnmx mentioned this pull request Jan 26, 2016
@arcnmx arcnmx force-pushed the nixstring-errno branch 2 times, most recently from 12ef980 to c437533 Compare January 26, 2016 03:27
@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 26, 2016

So as it turns out, #247 makes up the bulk of the changes. c437533 is the change on top of that.

@kamalmarhubi
Copy link
Member

@arcnmx since #247 (finally!) got merged, could you drop those commits from this PR and change its title?

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 28, 2016

Rebased. The title still applies.

@arcnmx
Copy link
Contributor Author

arcnmx commented Jan 28, 2016

Note that the cstr module is mostly for concenience to ease the transition, and used in the tests for that purpose. I'd suggest maybe pulling it out as a separate crate rather than having it built into nix proper.

@kamalmarhubi kamalmarhubi mentioned this pull request Feb 15, 2016
@homu
Copy link
Contributor

homu commented Feb 20, 2016

☔ The latest upstream changes (presumably #271) made this pull request unmergeable. Please resolve the merge conflicts.

@Susurrus
Copy link
Contributor

Susurrus commented Dec 4, 2017

Considering how old this is and how it looks like at least some of this was merged in #247, is there anything worth salvaging from this PR and someone willing to push this work forward? If not, let's go ahead and close this.

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 4, 2017

The PR still applies, although I'm not sure whether any conclusions were come to re #221. I'm happy to help update/rebase if a particular design is agreed on as the best way forward. This particular PR makes every API call just AsRef<CStr>, but there are other options/alternatives as discussed in that issue.

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.

5 participants