jacobwilliams / json-fortran

A Modern Fortran JSON API
https://jacobwilliams.github.io/json-fortran/
Other
332 stars 83 forks source link

Minor changes for nvfortran #496

Closed vyu16 closed 2 years ago

vyu16 commented 2 years ago

Hello,

When compiling JSON-Fortran with the nvfortran compiler, I ran into the following errors:

1.

NVFORTRAN-S-0026-Unmatched quote (/home/yu/opt/json-fortran/src/json_parameters.F90: 61)

Change: Partly reverted commit 90fee68f02.

2.

NVFORTRAN-S-0188-Argument number 2 to add_to_path: type mismatch (/home/yu/opt/json-fortran/src/json_value_module.F90: 7875)

Change: Removed an optional argument that is not used anyway.

With these changes, JSON-Fortran compiles with nvfortran 21.5 and 21.7.

Feel free to close if support for nvfortran is not of interest.

Thanks!

jacobwilliams commented 2 years ago

note: pushed a minor change just to see if that would make the CI run.

jacobwilliams commented 2 years ago

So, both of these look like nvfortran compiler bugs. (can you report them to nvidia?)

The first one I'm ok with merging. But, I think the one where you remove the CK_'' string is going to break something. Looking at the code, it looks like there is a reason for these blank strings, but I need to look into it more.

codecov-commenter commented 2 years ago

Codecov Report

Merging #496 (1169569) into master (90893ad) will not change coverage. The diff coverage is 75.00%.

:exclamation: Current head 1169569 differs from pull request most recent head bcf8646. Consider uploading reports for the commit bcf8646 to get more accurate results

@@           Coverage Diff           @@
##           master     #496   +/-   ##
=======================================
  Coverage   89.48%   89.48%           
=======================================
  Files           6        6           
  Lines        5353     5353           
=======================================
  Hits         4790     4790           
  Misses        563      563           
vyu16 commented 2 years ago

Thanks! I think (hope) the changes wouldn't break anything. In add_to_path, path_sep is only used in case(1_IK). I removed CK_'' from case(3_IK) and case(2_IK).

vyu16 commented 2 years ago

Found a workaround for the backslash issue: Use the -Mbackslash flag for nvfortran, and the -qnoescape flag for IBM Fortran. According to Google, backslash was not part of the character set in Fortran until Fortran 2003. The behavior was, and apparently still is, compiler dependent.

vyu16 commented 2 years ago

Any update on this pull request?

vyu16 commented 2 years ago

Hi @jacobwilliams , anything I can do to get this one merged? Thanks.

vyu16 commented 2 years ago

Update: I think the second point has been addressed by commit 9524515119e420e8dbc856031388c17f76d44a6b.

I updated the pull request to only fix the first point. As mentioned above, without the "fix", the -Mbackslash flag must be used to compile the code with nvfortran. Similarly, the -qnoescape flag is needed for xlf.