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

AbsolutePath::Parent should not re-check the path's absoluteness #17

Closed
ForNeVeR opened this issue Apr 21, 2024 · 2 comments · Fixed by #65
Closed

AbsolutePath::Parent should not re-check the path's absoluteness #17

ForNeVeR opened this issue Apr 21, 2024 · 2 comments · Fixed by #65
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ForNeVeR
Copy link
Owner

ForNeVeR commented Apr 21, 2024

Current implementation of AbsolutePath::Parent just creates a new AbsolutePath instance, which will perform a check for path's absoluteness (absolution?).

We should consider optimizing that case, since a parent of a known absolute path is guaranteed to be absolute itself (if it is present at all).

Perhaps introduce an private method to create instances of AbsolutePath unchecked?

See TODO[#17] in the code when implementing this.

@ForNeVeR ForNeVeR added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Apr 21, 2024
@ForNeVeR ForNeVeR changed the title AbsoilutePath::Parent should not re-check the path's absoluteness AbsolutePath::Parent should not re-check the path's absoluteness Apr 21, 2024
@ronimizy
Copy link
Contributor

i think that there may be a designated private constructor, that has a parameter specifying whether the absoluteness of the path should be checked, with signature like so:

private AbsolutePath(string path, bool checkAbsolutenness)

thus all existing constructors will invoke it with flag set to true and when we create a path in Parent property it would be set to false

does this solution align with your vision for the library?

@ForNeVeR
Copy link
Owner Author

Yeah, that sounds okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants