-
Notifications
You must be signed in to change notification settings - Fork 38
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
Remove need for non-standard SIZEOF #89
Remove need for non-standard SIZEOF #89
Conversation
ENDIF | ||
|
||
IF( ICLONELEN > 0 ) DEALLOCATE(ZCLONEA(IMLOC)%COMMSBUF) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that ICLONELEN could be unininitialized in case
IF( .NOT.ALLOCATED(ZCLONEA(IMLOC)%COMMSBUF) )THEN
(line 868) is not triggered.
Probably the IF(ALLOCATED(ZCLONEA(IMLOC)%COMMSBUF) ) THEN
was still correct.
Could it not also be that it needs to stay allocated across iterations? Otherwise why all the guards (line 868).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to do this can you just do:
IF( ALLOCATED(ZCLONEA) ) DEALLOCATE(ZCLONEA(IMLOC)%COMMSBUF)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @wdeconinck - we shouldn't use ICLONELEN
.
The first IF
has no purpose at all, as far as I can see. I can't see how it's possible for ZCLONEA(IMLOC)%COMMSBUF
to not be allocated at line 877.
Instead of checking if ICLONELEN > 0
, why don't we check STORAGE_SIZE(ZCLONEA(IMLOC)%COMMSBUF) > 0
? STORAGE_SIZE
is the same as SIZEOF
but returns the bits allocated rather than bytes, and it appears to be part of the Fortran 2008 standard, unlike SIZEOF
.
src/trans/internal/suleg_mod.F90
Outdated
! ZCLONES(IMLOC)%COMMSBUF=>NULL() | ||
ENDIF | ||
|
||
IF( ICLONELEN > 0 ) DEALLOCATE(ZCLONES(IMLOC)%COMMSBUF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
I've tested here and it seems to work for me. |
@DJDavies2 - could you test the latest commit with NAG Fortran? |
Given we can use STORAGE_SIZE instead of SIZEOF, could we not use the original code with just this change to avoid any surprising side effects? |
We could do that yes. I'll see what @DJDavies2 gets with NAG then update the PR. |
This branch seems to work for me as far as it goes. |
I'm down as a reviewer for this but I don't think I have the permissions to do that? I am happy to "approve" this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me. Nice cleanup
Builds on #78 and resolves #77.
I remove one redundant
IF
statement (right above there is logic which ensures the condition is always.TRUE.
.I also remove the
SIZEOF
by simply checking the value of the integer used to determine the size of the allocated array, rather than checking how many bytes were actually allocated.