Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rouson
Copy link
Contributor

@rouson rouson commented Jul 3, 2025

This PR

  1. Adds support for compiling with the LFortran compiler.
  2. Removes the diagnostic_data argument in assertions, which breaks backwards compatibility.

Copy link
Member

@bonachea bonachea left a 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.

Comment on lines -44 to -70
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
Copy link
Member

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.

Comment on lines -30 to +38
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", &
Copy link
Member

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.

Comment on lines +58 to +69
#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
Copy link
Member

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__)

Comment on lines +17 to +19
call assert(assertion = .true., description = "3 keyword arguments ")
call assert( .true., description = "2 keyword arguments ")
call assert( .true., "no optional argument")
Copy link
Member

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.

Suggested change
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")

rouson and others added 9 commits July 13, 2025 15:26
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.
@bonachea bonachea force-pushed the lfortran-and-simpler-assert branch from 3f9a433 to c736489 Compare July 13, 2025 19:26
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.

4 participants