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

Preliminary implementation of default values #73

Merged
merged 7 commits into from
Jan 4, 2020

Conversation

nshaffer
Copy link
Contributor

@nshaffer nshaffer commented Jan 3, 2020

In support of #62

  • New module "default_m" which exports the generic function name "default"
  • New entries in src/CMakeLists.txt and src/Makefile.manual

I've implemented a generic interface for real(sp), real(dp), real(qp), int16, int32, int64, logical, and character. I figure this is enough for a first pass. Complex types and discriminating between ASCII and UCS characters can come later. Probably no need to do specific logical kinds.

What's missing right now is tests. I don't know how to use CTest, but I would like to learn if anyone can link me a decent tutorial.

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.

Nice addition. Thanks. My comments are mainly associated with other PR or unresolved issues.

src/stdlib_experimental_default.f90 Outdated Show resolved Hide resolved
src/stdlib_experimental_default.f90 Outdated Show resolved Hide resolved
src/stdlib_experimental_default.f90 Outdated Show resolved Hide resolved
src/stdlib_experimental_default.f90 Outdated Show resolved Hide resolved
src/stdlib_experimental_default.f90 Outdated Show resolved Hide resolved
@certik
Copy link
Member

certik commented Jan 3, 2020

Thanks @nshaffer. I resolved the conflict with master and committed to your branch. Make sure you pull it before adding more commits.

Also, I recommend people to submit PRs from a different branch than their master, so that they can use their master to track the upstream master (to rebase, merge, etc.).

@certik
Copy link
Member

certik commented Jan 3, 2020

Regarding the test, try to copy let's say the test_ascii test and its CMake setup. It should be straightforward. If you have any questions, let us know.

nshaffer and others added 5 commits January 3, 2020 16:28
Co-Authored-By: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-Authored-By: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@nshaffer
Copy link
Contributor Author

nshaffer commented Jan 4, 2020

Thanks all for the comments. Seems like folks in #62 prefer the name "optval" so I converted to that everywhere and renamed the fallback argument "default", so that you can write optval(x, default=1.0) if you want to be explicit. Also wrote tests for all currently implemented types.

@certik
Copy link
Member

certik commented Jan 4, 2020 via email

Copy link
Member

@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.

This looks great. The API has the right order of arguments that everybody agrees, and optval seems to be the name that most also agree on. There are tests, implementation is clean. So let's merge it and start using it. For example in #71.

@certik certik merged commit affb52c into fortran-lang:master Jan 4, 2020
@certik
Copy link
Member

certik commented Jan 4, 2020

Thanks @nshaffer for the work!

@certik
Copy link
Member

certik commented Jan 4, 2020

Here is the first usage of optval: a8416a1

@nshaffer
Copy link
Contributor Author

nshaffer commented Jan 4, 2020

Cheers!

It occurs to me that optval should be pure elemental for the intrinsic types. Shall I tack that on here or open a new PR? (I've added a dev branch to my fork now, if that makes any difference.)

@certik
Copy link
Member

certik commented Jan 4, 2020

Yes, send another PR please against master, from a new branch.

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.

3 participants