-
Notifications
You must be signed in to change notification settings - Fork 9
Support LFortran and simplify assertions #61
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
base: main
Are you sure you want to change the base?
Conversation
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.
In today's meeting we decided to keep the scope of this PR unchanged (including both the removal of diagnostic_data
and the port to LFortran). Please resist appending any additional unrelated features.
I've migrated unresolved comments from PR #56.
block | ||
integer :: computed_checksum = 37, expected_checksum = 37 | ||
|
||
#if defined(_CRAYFTN) | ||
! Cray Fortran uses different line continuations in macro invocations | ||
call_assert_diagnose( computed_checksum == expected_checksum, & | ||
"Checksum mismatch failure!", & | ||
expected_checksum ) | ||
print *," passes with macro-style line breaks" | ||
|
||
call_assert_diagnose( computed_checksum == expected_checksum, & ! ensured since version 3.14 | ||
"Checksum mismatch failure!", & ! TODO: write a better message here | ||
computed_checksum ) | ||
print *," passes with C block comments embedded in macro" | ||
#else | ||
call_assert_diagnose( computed_checksum == expected_checksum, \ | ||
"Checksum mismatch failure!", \ | ||
expected_checksum ) | ||
print *," passes with macro-style line breaks" | ||
|
||
call_assert_diagnose( computed_checksum == expected_checksum, /* ensured since version 3.14 */ \ | ||
"Checksum mismatch failure!", /* TODO: write a better message here */ \ | ||
computed_checksum ) | ||
print *," passes with C block comments embedded in macro" | ||
#endif | ||
|
||
end block |
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.
This deleted test code was intended to test the original code in the top-level README which this PR has rewritten to use call_assert_describe
, but no corresponding replacement test code has been added.
I suggest we restore this block and apply the same edits applied to the code in README.md, to maintain test coverage for our documented code snippets.
command = "echo 'example/false_assertion.F90: unsupported compiler' && exit 1", & | ||
! For all other compilers, we assume that the default fpm command works | ||
command = "fpm run --example false-assertion --profile release --flag '-DASSERTIONS -ffree-line-length-0' > /dev/null 2>&1", & |
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.
This is an unsafe assumption.
As described in the block comment above, this test hard-codes fragile compiler-specific commands, and conflates command failure with test success. Thus the addition of every new compiler needs to be manually verified, and should never be defaulted in this manner.
Please revert this change and add a new block with a command has been verified for correctness with LFortran.
#else | ||
#ifdef __LFORTRAN__ | ||
print *,trim(merge("passes","FAILS ",exit_status/=0)) // " on error-terminating when assertion = .false." | ||
#else | ||
block | ||
integer unit | ||
open(newunit=unit, file="build/exit_status", status="old") | ||
read(unit,*) exit_status | ||
print *,trim(merge("passes","FAILS ",exit_status/=0)) // " on error-terminating when assertion = .false." | ||
close(unit) | ||
end block | ||
#endif | ||
#endif |
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.
This transformation was apparently done to add support for LFortran, but the change has also (inadvertently?) perturbed and broken the behavior for single-image execution on other compilers.
So for example, the following command no longer produces correct output:
fpm test --compiler gfortran --flag "-DASSERTIONS -DASSERT_MULTI_IMAGE=0"
The removed line needs to be restored, and the first two lines should instead be #elif defined(__LFORTRAN__)
call assert(assertion = .true., description = "3 keyword arguments ") | ||
call assert( .true., description = "2 keyword arguments ") | ||
call assert( .true., "no optional argument") |
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.
This block is outdated and misleading. There are no longer optional arguments, and there are at most 2 keyword arguments.
call assert(assertion = .true., description = "3 keyword arguments ") | |
call assert( .true., description = "2 keyword arguments ") | |
call assert( .true., "no optional argument") | |
call assert(assertion = .true., description = "2 keyword arguments") | |
call assert( .true., description = "1 keyword arguments") | |
call assert( .true., "0 keyword arguments") |
This commit 1. Removes the diagnostic_data argument from the assert subroutine 2. Removes the types that existed solely to support that argument: a. characterizable_t b. intrinsic_array_t 3. Edits or deletes examples that referenced the removed entities 4. Edits or deletes documentation that referenced removed entities
Also manually inline string function.
3f9a433
to
c736489
Compare
This PR
diagnostic_data
argument in assertions, which breaks backwards compatibility.