Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

stdlib_system: essential path functionality #999

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

Merged
perazz merged 20 commits into fortran-lang:master from wassup05:path
Jul 22, 2025
Merged

Conversation

@wassup05
Copy link
Contributor

@wassup05 wassup05 commented Jun 7, 2025

A few path related functions for ease of future functionality have been added.

  • joinpath: joins the given paths according to the platform's path-separator.
  • operator(/): as was suggested in the Fortran discourse here an operator is also provided for the same functionality
  • splitpath: splits the path following the last path-separator and returns the head and tail.
  • basename: just returns the tail of splitpath
  • dirname: just returns the head of splitpath

The pathsep parameter contains either / or \ depending on the platform and is a compile-time constant now, so is the parameter, ISWIN

Do let me know your thoughts.

jalvesz, sebastian-mutz, and zoziha reacted with rocket emoji
Copy link
Member

Thanks for the PR, @wassup05. Just a minor point (not related to functionality) about examples for now: It's always useful - esp. for those learning by examples - to have an idea of the expected output. Other stdlib examples add the expected print results as a comment below the respective commands. I would recommend to do the same here.

wassup05 reacted with thumbs up emoji

Copy link
Contributor Author

I will add that with the other reviews @sebastian-mutz, I don't want to trigger the ci/cd workflow just for a few comments. Also on a second thought maybe instead of the if (ISWIN) ... thing I could separate the two examples...

jalvesz and sebastian-mutz reacted with thumbs up emoji

Copy link
Contributor

jalvesz commented Jun 15, 2025

The issue with Ninja detecting a "circular dependency" (there was non) was related to the stdlib_system.F90 file not being visible in a correct manner in the CMakeLists.txt file.

The group:

# Preprocessed files to contain preprocessor directives -> .F90
set(cppFiles

corresponds to .fypp files for which the suffix should be changed to .F90. Not for already .F90 files.

wassup05 reacted with thumbs up emoji

Copy link
Contributor

@jalvesz jalvesz left a comment

Choose a reason for hiding this comment

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

LGTM @wassup05! Thanks for this nice work!!

wassup05 reacted with thumbs up emoji
Copy link
Contributor

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 introduces essential path manipulation functions and related operator overloads to simplify path handling across platforms. Key changes include:

  • Implementation of joinpath (and its operator(/)) for concatenating paths.
  • Addition of splitpath and its derivatives (basename, dirname) for decomposing paths.
  • Updates to tests, examples, documentation, and build files to support and illustrate the new functionality.

Reviewed Changes

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

Show a summary per file
File Description
test/system/test_path.f90 Adds unit tests for new path functions and operator functionality.
test/system/CMakeLists.txt Registers the new "path" test.
src/stdlib_system_path.f90 Implements joinpath, operator(/), splitpath, basename, and dirname.
src/stdlib_system.F90 Defines interfaces and exports for the new path functionality.
src/CMakeLists.txt Includes the new path source file in the build.
example/system/*.f90 Provides usage examples for joinpath, splitpath, dirname, and basename.
doc/specs/stdlib_system.md Updates the spec documentation to incorporate the new interfaces.
config/fypp_deployment.py Passes the uppercase system name flag to the build commands.
CMakeLists.txt Adds compile options to define the system name macro.
Comments suppressed due to low confidence (2)

example/system/example_path_dirname.f90:2

  • The program name 'example_path_splitpath' does not match the file's purpose (dirname functionality). Consider renaming it to 'example_path_dirname'.
program example_path_splitpath

example/system/example_path_basename.f90:2

  • The program name 'example_path_splitpath' in this file appears to be a copy-paste error; it should reflect basename functionality (e.g., 'example_path_basename').
program example_path_splitpath

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Nice start @wassup05, thank you for this PR. It's a bit long so I tried to give some initial comments first hand. Let's first discuss and iterate around these comments, and then we can start planning about more path functionality, thank you.

Comment on lines 96 to 100
public :: joinpath
public :: operator(/)
public :: splitpath
public :: basename
public :: dirname
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add short FORD comment blocks for theese APIs? As an example:

!! version: experimental
!!
!! Tests if a given path matches an existing directory.
!! ([Specification](../page/specs/stdlib_system.html#is_directory-test-if-a-path-is-a-directory))
!!
!!### Summary
!! Function to evaluate whether a specified path corresponds to an existing directory.
!!
!!### Description
!!
!! This function checks if a given file system path is a directory. It is cross-platform and utilizes
!! native system calls. It supports common operating systems such as Linux, macOS,
!! Windows, and various UNIX-like environments. On unsupported operating systems, the function will return `.false.`.
!!
public :: is_directory

Copy link
Contributor

@jalvesz jalvesz Jun 19, 2025
edited
Loading

Choose a reason for hiding this comment

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

If the documentation is added here, it should be placed following the interface declaration or within the function declaration if there is no additional interface, and not before the public statement as it is done within this module currently. I actually have issues with FORD locally to produce the documentation because of that but haven't mention it yet to avoid mixing issues within the same PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is a good idea to address (these and perhaps more of) FORD issues in a separate PR. For the current PR, I think it's ok to stay consistent with the current syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the docs following the interface blocks of these interfaces... Isn't that enough?

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Thank you for implementing these updates @wassup05.

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @wassup05, I have left a few final comments.


### Arguments

`p1, p2`: Shall be a character string. It is an `intent(in)` argument.
Copy link
Member

Choose a reason for hiding this comment

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

Because stdlib offers a string type, it would be nice if these string manipulation functions were interfaces working with either character strings (as currently proposed), or the internal string_type type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made these functions interfaces keeping this possibility in mind... But what is considered a good way of doing this? Writing procedures for string_type and then, when calling them with character variables just assign these to the new string_type variables and call the string_type version of the interface itself or code duplication, keeping the code separate for these two although they are in essence the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I got it!

perazz reacted with rocket emoji
Copy link
Member

Choose a reason for hiding this comment

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

Yes: the easiest option is to keep the character implementation and then return the result to a string via move:

!> Moves the allocated character scalar from 'from' to 'to'
!> [Specifications](../page/specs/stdlib_string_type.html#move)
interface move
module procedure :: move_string_string
module procedure :: move_string_char
module procedure :: move_char_string
module procedure :: move_char_char
end interface move

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the relevant changes... But it doesn't use move, it uses assignment(=) and char, Let me know what you think of it.

Copy link
Member

@perazz perazz Jul 15, 2025
edited
Loading

Choose a reason for hiding this comment

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

It works @wassup05, however, there are two copies: string->char for the input arguments, and then char -> string on the assignment. Using move would at least avoid incurring the last copy. Just do:

character(:), allocatable :: join_char
join_char = join_path(char(a),char(b))
call move(from=join_char,to=...result variable...)

wassup05 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Because the internal string_type representation is just an allocatable character variable, I would also support (to be discussed with the community as a separate PR) the implementation of a tiny zero-copy "view" function for the string type, see here:

 !> Zero-copy view of the string as a character array
 pure function view(string) result(char_array)
 type(string_type), intent(in), target :: string
 character(:), pointer :: char_array
 if (allocated(string%raw)) then
 char_array => string%raw
 else
 nullify(char_array)
 end if
 end function view

wassup05 reacted with heart emoji
Copy link
Member

perazz commented Jul 22, 2025

In absence of further comments, I will merge this one, thank you.

@perazz perazz merged commit a0d9e22 into fortran-lang:master Jul 22, 2025
17 of 18 checks passed
@wassup05 wassup05 deleted the path branch July 22, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jalvesz jalvesz jalvesz approved these changes

Copilot code review Copilot Copilot left review comments

@perazz perazz perazz approved these changes

@jvdp1 jvdp1 Awaiting requested review from jvdp1

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /