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

String list new #680

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

Conversation

arjenmarkus
Copy link
Member

No description provided.

The API for the module to manipulate lists of strings has been discussed and this has resulted in the current implementation
There was a typo in some comments - corrected
Document the linked_list type, including examples, and correct a few minor mistakes in the code.
Correct the subdirectory for the examples in the documentation.
Move the actual implementations of the linked list type (the linked list itself and the child list component) to the directory src.
Use an include statement instead of relying on the automatic module identification to get access to the print_list subroutine. As it is merely an auxiliary, it should not become part of the "official" source code.
Use an internal routine for print_list instead of an included module, as the buil process still wants to find the module's source.
Add the subdirectory "linked_list" to the overall CMake file and add a specific CMake file for the examples therein.
Rename the examples so that the names of the targets do not conflict with existing examples for other modules (notably in the strings subdirectory
Use a dedicated macro to make sure that the include directory is registered per target. It did not show up in the earlier attempt.
@@ -0,0 +1,17 @@
gfortran -c ../../src/stdlib_child_list.f90
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this file should be removed from the directory.

@LKedward
Copy link
Member

LKedward commented Sep 21, 2022

I think the fpm-deployment CI is failing because you need to update the ci/fpm-deployment.sh script to also copy over the included files into an ./include folder in the root directory.

Correct the CMakeLists.txt file in the src directory to include the linked list modules , also adjust the fpm-deployment.sh to copy any auxiliary source files from the example directory.
Rename the include file to "*.inc" so that it is no longer recognised by the build system. Of course, the example source files had to be adapted as well.
Correct the performance test program - apparently it was using some deprecated names and it was not built using the CMake build system.
@arjenmarkus
Copy link
Member Author

Thanks for the comments. I was not sure how I could introduce an include directory, so instead I simply copy the extra file in to the example directory. I had to rename the file to something that is not confused with a separate Fortran program. Also I had to tweak the CMakeLists.txt files for the test program. I seem to be almost there, but the test program fails to run properly. Looking into that now.

Update the test program to NOT read from stdin - that does not work in the CI build and execute environment.
@arjenmarkus
Copy link
Member Author

The test program was failing because it tried to read from the keyboard. Changed it to simply use a length of one million elements.

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 for this progress @arjenmarkus . Here are some suggestions.
Note that we use the suffix _type for derived types in stdlib.

doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linked_list.md Outdated Show resolved Hide resolved
---
title: stringlist
---
# Lists of strings
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference with the stdlib stringlist_type?

@arjenmarkus
Copy link
Member Author

arjenmarkus commented Dec 28, 2023 via email

Updated the documentation after review by jvdp1. Adjusted the type name (suffix _type) and applied several cosmetic changes. Also added a more extensive test program and fixed several bugs.
As the type is now "linked_list_type", the examples had to be adjusted. There turned out to be a bug regarding the intent for the get() function. This has been repaired.
The file "linked_list_aux.inc" was not found - fpm test.
Something went wrong with the included routine in the examples. So instead, simply copy the source code for it in all examples.
@arjenmarkus
Copy link
Member Author

arjenmarkus commented Dec 28, 2023 via email

@arjenmarkus
Copy link
Member Author

arjenmarkus commented Dec 28, 2023 via email

@jvdp1
Copy link
Member

jvdp1 commented Dec 29, 2023

Two failures: on MSYS2 CMake was not found, on Ubuntu latest ifort was not found. How should we resolve these problems?

Such issues should have been solved with one of the latest PR. The CI seem to be ok now.

module procedure stringlist_index_subtract
end interface

type string_type
Copy link
Member

Choose a reason for hiding this comment

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

could the string_type present in stdlib_string_type be used instead of redefining a new one?

@arjenmarkus
Copy link
Member Author

arjenmarkus commented Jan 2, 2024 via email

@arjenmarkus
Copy link
Member Author

arjenmarkus commented Jan 2, 2024 via email

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.

None yet

3 participants