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

Time for polymorphism? #405

Open
jacobwilliams opened this issue Jun 23, 2019 · 4 comments · May be fixed by #412
Open

Time for polymorphism? #405

jacobwilliams opened this issue Jun 23, 2019 · 4 comments · May be fixed by #412

Comments

@jacobwilliams
Copy link
Owner

Originally, when I was creating this code by updating the old FSON code, I made the data in json_value polymorphic. But, I had to get rid of that at the time because of some compiler bug (I think it was a memory leak in ifort). Currently, each json_value contains all possible types it can hold and we just allocate the one we need:

 real(RK),allocatable                 :: dbl_value  !! real data for this variable
 logical(LK),allocatable              :: log_value  !! logical data for this variable
 character(kind=CK,len=:),allocatable :: str_value  !! string data for this variable
                                                    !! (unescaped)
 integer(IK),allocatable              :: int_value  !! integer data for this variable

 integer(IK) :: var_type = json_unknown  !! variable type

I never liked this though. Maybe it's time to revisit this.

@zbeekman
Copy link
Contributor

My only fear is allocatable deferred length characters. But, it would be great to use polymorphism.

@jacobwilliams
Copy link
Owner Author

First cut is done! Seems to pass all the unit tests.

@zbeekman In your copious free time :), if you have any comments I would be interested to hear them. I'm going to wait a while to merge this in until I look over it some more.

Note: With these changes, I think I will revisit #38. For some use cases, it would be way more efficient to just store arrays as normal fortran arrays rather than as linked lists. Example:

    type,extends(json_data) :: json_real_vec1d_type
        !! a `json_real` 1d array
        real(RK),dimension(:),allocatable :: value
    end type json_real_vec1d_type

I'd need to think through the implications of this for the parser, etc...

For people using (abusing?) JSON-Fortran to collect and access large array data sets (but don't care about being able to manipulate the JSON structure) it would be a massive improvement I think.

@zbeekman
Copy link
Contributor

I will be extending JSON-Fortran this month (starting today/tomorrow and next week) to have native support for ragged-edge matrices. So long as arrays of arrays are still handled well (I don't see why they wouldn't be the case) then this should be fine.

As previously mentioned, I'm a bit concerned about allocatable character scalars (and how will you treat arrays of strings?) because they seem to trigger strange bugs with GFortran and (to a lesser extend not) Intel. The project I am working on requires Intel 18 (windows & linux) and GFortran 8.3 (macOS, Linux). So long as complicated cases work with these toolchains (with and w/o optimization enabled), I'd be happy to see this implemented.

@jacobwilliams
Copy link
Owner Author

jacobwilliams commented Jul 12, 2019

Awesome! Yes, I think it would work for arrays of arrays if done right.

Note: I added (#414) more versions of gfortran to the travis tests. So we can test all the recent versions. Note that I still have many compiler directives in the code to workaround various gfortran bugs for allocatable chars. Probably now that I'm testing all the versions, I can see about only applying the workarounds to the versions that still have the bug. Probably that would speed it up a bit too (#415).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants