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

Improved support for NAG #418

Merged
merged 7 commits into from
Jun 3, 2021
Merged

Improved support for NAG #418

merged 7 commits into from
Jun 3, 2021

Conversation

sakamoti
Copy link
Contributor

@sakamoti sakamoti commented May 28, 2021

Because of nagfor's strict error checking characteristics, execution is halted by default when a floating point exception occurs. Some of the runtime errors can be resolved by adding the option '-ieee=full'.
Other errors have been corrected to conform more strictly to the fortran standard.
These commit still leaves the one runtime error 'string_derivedtype_io'.
#108

test_mean_f03.f90 cause floating invalid operation error because some
variable is not initialized.
Optional argument value has been passed for the first argument of merge
routine. some compiler can't allow such situation.
The Fortran standard says "The value of a file-unit-number shall
identify a valid unit.".
Once the newunit value generated by open(newunit=..) is not
connected to a unit, it is not valid.
When the unit number is not valid, the compiler can do anything
from return .TRUE., .FALSE., raise an error. Therefore, I added the iostat
argument for the inquire routine.
@sakamoti sakamoti changed the title Improved support for NAG #108 Improved support for NAG May 28, 2021
@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label May 30, 2021
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks for sharing. This looks good to me, pending a some minor comments and a few whitespace nitpicks.

src/stdlib_logger.f90 Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/tests/logger/test_stdlib_logger.f90 Outdated Show resolved Hide resolved
src/tests/stats/test_mean_f03.f90 Outdated Show resolved Hide resolved
src/tests/logger/test_stdlib_logger.f90 Outdated Show resolved Hide resolved
src/stdlib_logger.f90 Outdated Show resolved Hide resolved
sakamoti and others added 3 commits May 30, 2021 20:39
Adding white spaces

Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
Delete unused character.
Changed to work even when string size is 0.
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
src/stdlib_string_type.fypp Outdated Show resolved Hide resolved
ichar(" ") should be alloed.
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.

LGTM, pending a question.
Thank you.

Comment on lines +424 to +428
if (allocated(string%raw) .and. len(string) > 0) then
ich = ichar(string%raw(1:1))
else
ich = 0
end if
Copy link
Member

Choose a reason for hiding this comment

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

May I ask what was the issue with the original code?

Copy link
Contributor Author

@sakamoti sakamoti May 30, 2021

Choose a reason for hiding this comment

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

Accordings to the book 'Modern Fortran explained (Incorporating Fortran 2018)', argument of 'ichar' is a character.
'string%raw' can be more than length one. In such case, runtime error occurred when use nagfor. On the other hands, gfortran seems to accept string argument.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @sakamoti for your answer. But I was wondering why the merge option was removed.

Copy link
Member

Choose a reason for hiding this comment

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

merge doesn't necessarily short-circuit the evaluation of its arguments like a ternary operator in C would. This could lead to an access of the uninitialized raw payload of the string_type which is an error.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @awvwgk for the explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I havn't know which one is more efficiently to select 'merge' or 'if construct'. I just felt a bit complex to write with 'merge'. And I want to acces 'string%raw' only when 'conditions' becoms true. So, I select to write with 'if construct'.
Thank you @awvwgk for the explanation.

@awvwgk awvwgk added compilers and removed reviewers needed This patch requires extra eyes labels May 30, 2021
@awvwgk awvwgk merged commit 4b96cdd into fortran-lang:master Jun 3, 2021
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