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

Importing issues with requirements and templates #3147

Merged
merged 19 commits into from
Jan 25, 2024

Conversation

ansharlubis
Copy link
Contributor

@ansharlubis ansharlubis commented Jan 20, 2024

Addressing issues #3139 and #3138.

The following files make up the matrix example in separate modules:

  • template_semigroup.f90
  • template_monoid.f90
  • template_semiring.f90
  • template_unitring.f90
  • template_field.f90
  • template_matrix.f90
  • template_matrix_real.f90
  • template_matrix_integer.f90

@ansharlubis ansharlubis added the generics Fortran 202Y Generics label Jan 20, 2024
@ansharlubis ansharlubis changed the title Importing issues with Requirements and Templates Importing issues with requirements and templates Jan 20, 2024
@@ -7,6 +7,7 @@
#include <libasr/serialization.h>
#include <libasr/bwriter.h>

#include <libasr/pickle.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be needed here.

@certik
Copy link
Contributor

certik commented Jan 20, 2024

Some of the new reference tests are 14K of lines, which will increase the size of the git repository. Do we need to check all that? Or can we just run it via integration tests.

@certik
Copy link
Contributor

certik commented Jan 20, 2024

Does it make sense to keep the original all-in-one-file test (to ensure that still works), as well as the new split tests?

@certik
Copy link
Contributor

certik commented Jan 20, 2024

Other than that, the changes look good to me.

@ansharlubis
Copy link
Contributor Author

ansharlubis commented Jan 21, 2024

@certik Some of the tests are superfluous, I want to combine them first.

For the matrix example, just the combined one should be enough. Integration test for the split files should be enough. However, I don't know how to add split files into the integration tests.

@certik
Copy link
Contributor

certik commented Jan 21, 2024

Look at the modules_15 test, that contains several files as well. You probably need to add a main program, so that it creates an executable that can be run.

@ansharlubis
Copy link
Contributor Author

I follow the other example, added the extra files into integration_tests/CMakeLists.txt and the main program but they still don't link properly.

@certik
Copy link
Contributor

certik commented Jan 24, 2024

We have to debug it.

@@ -564,6 +564,51 @@ static inline std::string type_to_str(const ASR::ttype_t *t)
}
}

static inline std::string type_to_str_with_substitution(const ASR::ttype_t *t,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document what exactly this function does and how it differs to the other similar function for type comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I'll add the documentation in a separate pull request.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is good enough to merge. Left a comment to improve docs.

@certik certik merged commit 5650607 into lfortran:main Jan 25, 2024
21 checks passed
@ansharlubis ansharlubis deleted the generics-matrix-issue branch March 4, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generics Fortran 202Y Generics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants