Skip to content

Conversation

elinor-fung
Copy link
Member

See #116597 (comment)

Make append_path a simple concatenation, similar to Path.Join. The existing behaviour around rooting is not what the usage actually needs/wants.

cc @dotnet/appmodel @AaronRobinsonMSFT

@Copilot Copilot AI review requested due to automatic review settings July 7, 2025 22:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies append_path by removing rooted-path handling and making it a straightforward concatenation with separator management.

  • Removed rooting logic and added no-op for empty path2
  • Ensured correct separator insertion only when needed
  • Preserved behavior when path1 is empty

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/native/corehost/hostmisc/utils.h Updated comment for append_path to reflect simple concatenation
src/native/corehost/hostmisc/utils.cpp Simplified append_path implementation and edge checks
Comments suppressed due to low confidence (2)

src/native/corehost/hostmisc/utils.cpp:53

  • Add unit tests for append_path covering key scenarios: empty path2, empty path1, existing trailing separator in path1, and leading separator in path2 to verify all branches work as intended.
void append_path(pal::string_t* path1, const pal::char_t* path2)

src/native/corehost/hostmisc/utils.h:80

  • [nitpick] Update this comment to note that an empty path2 is a no-op and that no-rooted checks are performed, aligning with the new behavior.
// Concatenate path1 and path2 into path1, ensuring there is a directory separator.

Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

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.

2 participants