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

Swap #869

Merged
merged 18 commits into from
Sep 24, 2024
Merged

Swap #869

merged 18 commits into from
Sep 24, 2024

Conversation

jalvesz
Copy link
Contributor

@jalvesz jalvesz commented Sep 8, 2024

Proposal to add an elemental swap interface for base kinds

  • elemental subroutines
  • tests
  • examples
  • documentation

Reference
https://fortranwiki.org/fortran/show/swap by @urbanjost

cc: @jvdp1 @perazz

@perazz
Copy link
Member

perazz commented Sep 8, 2024

I think this is a great useful addition @jalvesz.
The only question I have is about the string version. Consider this example:

program tswap
    character(5) :: a = '12345'
    character(8) :: b = 'abcdefgh'
    print *, a, ' <-> ', b
    call swap_str(a,b)
    print *, a, ' <-> ', b
contains
    elemental subroutine swap_str(lhs,rhs)
        character(*), intent(inout) :: lhs, rhs
        character(len=max(len(lhs),len(rhs))) :: temp
        temp = lhs ; lhs = rhs ; rhs = temp
    end subroutine
end

that prints:

 12345 <-> abcdefgh
 abcde <-> 12345

An option to overcome this issue, would be to fix the string length:

character(*), intent(inout) :: lhs
character(len=len(lhs)), intent(inout) :: res

and then provide an interface for the stdlib_string_type maybe?

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks for sharing. Can you explicitly test whether swap(x, x) works as expected?

@jalvesz
Copy link
Contributor Author

jalvesz commented Sep 8, 2024

@perazz I think that the limitation here is that this swap interface is intended as a "swap values" under the hypothesis of equal kind and type lhs and rhs. In order to accomplish a dynamic swap, we would need to swap the memory address, something like:

program tswap
    character(:), allocatable :: a 
    character(:), allocatable :: b
    a = '12345'
    b = 'abcdefgh'
    print *, 'Swap values'
    print *, a, ' <-> ', b
    call swap_str(a,b)
    print *, a, ' <-> ', b

    a = '12345'
    b = 'abcdefgh'
    print *, 'Swap mem address'
    print *, a, ' <-> ', b
    call swap_str_a(a,b)
    print *, a, ' <-> ', b
contains
    elemental subroutine swap_str(lhs,rhs)
        character(*), intent(inout) :: lhs, rhs
        character(len=max(len(lhs),len(rhs))) :: temp
        temp = lhs ; lhs = rhs ; rhs = temp
    end subroutine
    subroutine swap_str_a(lhs,rhs)
        character(:), allocatable, intent(inout) :: lhs, rhs
        character(:), allocatable :: temp
        call move_alloc( lhs , temp )
        call move_alloc( rhs , lhs )
        call move_alloc( temp , rhs )
    end subroutine
end

Now, these two procedures cannot be interfaced under the same interface as it leads to an ambiguous interface. But a secondary interface can be added to handle this use-case, like mem_swap or something in that line?

We could propose also a swap interface to the stdlib_string_type indeed.

@jalvesz
Copy link
Contributor Author

jalvesz commented Sep 8, 2024

@awvwgk adding something like this ? :

subroutine test_swap_${k1}$(error)
        type(error_type), allocatable, intent(out) :: error
        ${t1}$ :: x(3), y(3)
        
        x = [${t1}$ :: 1, 2, 3]
        y = [${t1}$ :: 4, 5, 6]

        call swap(x,y)
        
        call check(error, all( x == [${t1}$ :: 4, 5, 6] ) )
        if (allocated(error)) return
        call check(error, all( y == [${t1}$ :: 1, 2, 3] ) )
        if (allocated(error)) return

        ! check self swap
        call swap(x,x)
        
        call check(error, all( x == [${t1}$ :: 4, 5, 6] ) )
        if (allocated(error)) return
end subroutine test_swap_${k1}$

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me

@perazz
Copy link
Member

perazz commented Sep 9, 2024

We could propose also a swap interface to the stdlib_string_type indeed.

Exactly @jalvesz, so the stdlib user has one more reason to use the provided string type.

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.

Looks great as far as I'm concerned @jalvesz, thank you.

Copy link
Member

@jvdp1 jvdp1 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 @jalvesz . LGTM. Here are some minor comments.

Copy link
Member

@jvdp1 jvdp1 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 @jalvesz . LGTM. I left a few minor comments/suggestions. Feel free to ignore them.

@perazz
Copy link
Member

perazz commented Sep 18, 2024

I see that there are no further comments @jvdp1 @jalvesz @awvwgk, should we wait a couple more days and then merge?

@jalvesz
Copy link
Contributor Author

jalvesz commented Sep 18, 2024

I've added a warning and extended the example specific to characters in 1119964

@jalvesz
Copy link
Contributor Author

jalvesz commented Sep 24, 2024

I think this PR is ready to be merged if there are no further comments?

@perazz
Copy link
Member

perazz commented Sep 24, 2024

With 3 approvals and no further comments, I will merge this.

@perazz perazz merged commit c663dc1 into fortran-lang:master Sep 24, 2024
16 checks passed
@jalvesz jalvesz deleted the swap branch September 24, 2024 12:30
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