Skip to content

Conversation

krystophny
Copy link
Collaborator

@krystophny krystophny commented Oct 3, 2025

User description

Summary

  • add zlib_compress_into subroutine to avoid returning large allocatables from functions
  • keep existing zlib_compress function as wrapper for backwards compatibility
  • update raster/pdf backends to call the new helper and add optional debug logging

Testing

  • fpm test

PR Type

Bug fix, Enhancement


Description

  • Add zlib_compress_into subroutine to avoid returning large allocatables

  • Keep existing zlib_compress function as backwards-compatible wrapper

  • Update PNG and PDF backends to use new helper

  • Add optional debug logging with environment variable control


Diagram Walkthrough

flowchart LR
  A["zlib_compress function"] --> B["zlib_compress_into subroutine"]
  B --> C["PNG backend"]
  B --> D["PDF backend"]
  B --> E["Debug logging"]
  A --> F["Backwards compatibility wrapper"]
Loading

File Walkthrough

Relevant files
Enhancement
fortplot_zlib_core.f90
Add safer compression subroutine with debug logging           

src/external/fortplot_zlib_core.f90

  • Add zlib_compress_into subroutine that allocates output buffer
    directly
  • Convert existing zlib_compress to wrapper calling new subroutine
  • Add debug logging infrastructure with environment variable control
  • Add utility functions for environment parsing and case conversion
+117/-24
fortplot_zlib.f90
Export new compression subroutine in bridge module             

src/external/fortplot_zlib.f90

  • Add zlib_compress_into to module exports
  • Update public interface to include both old and new compression
    functions
+3/-3     
Bug fix
fortplot_png.f90
Update PNG backend to use new compression subroutine         

src/backends/raster/fortplot_png.f90

  • Update import to use zlib_compress_into instead of zlib_compress
  • Replace function call with subroutine call in generate_png_data
+2/-2     
fortplot_pdf.f90
Update PDF backend to use new compression subroutine         

src/backends/vector/fortplot_pdf.f90

  • Update import to use zlib_compress_into instead of zlib_compress
  • Replace function call with subroutine call in heatmap wrapper
+2/-2     
fortplot_pdf_io.f90
Update PDF I/O to use new compression subroutine                 

src/backends/vector/fortplot_pdf_io.f90

  • Update import to use zlib_compress_into instead of zlib_compress
  • Replace function call with subroutine call in write_content_object
+2/-2     

Copy link

qodo-merge-pro bot commented Oct 3, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Debug info leakage

Description: Debug logging prints directly to stdout via print when the environment flag is enabled,
which can inadvertently expose sensitive runtime data in logs; consider routing through
the project's logging facility with appropriate levels.
fortplot_zlib_core.f90 [1047-1053]

Referred Code
subroutine log_zlib_debug(message)
    !! Emit debug message when instrumentation enabled
    character(len=*), intent(in) :: message
    if (is_zlib_debug_enabled()) then
        print '(a)', '[fortplot:zlib] '//trim(message)
    end if
end subroutine log_zlib_debug
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

qodo-merge-pro bot commented Oct 3, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent race conditions during initialization

To prevent a race condition in multi-threaded environments, make the
ensure_zlib_debug subroutine thread-safe by wrapping the initialization logic in
an atomic block.

src/external/fortplot_zlib_core.f90 [1025-1039]

 subroutine ensure_zlib_debug()
     !! Lazy initialization for instrumentation flag
     character(len=32) :: env_value
     integer :: status
 
     if (zlib_debug_initialized) return
 
-    call get_environment_variable('FORTPLOT_ZLIB_DEBUG', env_value, status=status)
-    if (status == 0) then
-        zlib_debug_enabled_state = parse_debug_env(env_value)
-    else
-        zlib_debug_enabled_state = .false.
+    atomic
+    if (.not. zlib_debug_initialized) then
+        call get_environment_variable('FORTPLOT_ZLIB_DEBUG', env_value, status=status)
+        if (status == 0) then
+            zlib_debug_enabled_state = parse_debug_env(env_value)
+        else
+            zlib_debug_enabled_state = .false.
+        end if
+        zlib_debug_initialized = .true.
     end if
-    zlib_debug_initialized = .true.
+    end atomic
 end subroutine ensure_zlib_debug

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential race condition in the newly added ensure_zlib_debug subroutine, which is a significant correctness issue in multi-threaded contexts, and proposes a valid fix.

Medium
High-level
Consolidate logging into the existing module

The PR adds a new debug logging system in fortplot_zlib_core.f90. It is
recommended to integrate this functionality into the existing fortplot_logging
module to ensure a consistent logging approach across the project.

Examples:

src/external/fortplot_zlib_core.f90 [987-1053]
    pure function to_lower_char(ch) result(lower)
        !! Lower-case conversion for ASCII characters
        character(len=1), intent(in) :: ch
        character(len=1) :: lower
        integer :: code

        lower = ch
        code = iachar(ch)
        if (code >= iachar('A') .and. code <= iachar('Z')) then
            lower = achar(code + 32)

 ... (clipped 57 lines)

Solution Walkthrough:

Before:

! in fortplot_zlib_core.f90
subroutine zlib_compress_into(...)
    logical :: debug_active
    ...
    debug_active = is_zlib_debug_enabled()
    if (debug_active) then
        call log_zlib_debug(...)
    end if
    ...
end subroutine

subroutine log_zlib_debug(message)
    if (is_zlib_debug_enabled()) then
        print '(a)', '[fortplot:zlib] '//trim(message)
    end if
end subroutine

logical function is_zlib_debug_enabled()
    ! reads FORTPLOT_ZLIB_DEBUG env var
    ...
end function

After:

! in fortplot_zlib_core.f90
use fortplot_logging, only: log_debug

subroutine zlib_compress_into(...)
    ...
    call log_debug('[fortplot:zlib] zlib_compress_into begin...')
    ...
end subroutine


! in fortplot_logging.f90
subroutine log_debug(message)
    if (is_debug_enabled()) then
        print '(a)', '[DEBUG] ' // trim(message)
    end if
end subroutine

logical function is_debug_enabled()
    ! reads a general debug env var, e.g. FORTPLOT_LOG_LEVEL
    ...
end function
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies the introduction of a parallel logging system and rightly proposes to integrate it with the existing fortplot_logging module for better design consistency and maintainability.

Medium
General
Correctly handle empty environment variables
Suggestion Impact:The commit removed the local parse_debug_env and related debug handling in this module, replacing it with shared debug utilities. This eliminates the prior behavior where an empty env var was treated as true, aligning with the suggestion’s intent.

code diff:

@@ -983,72 +982,5 @@
         end do
     end function bit_reverse
 
-    pure function to_lower_char(ch) result(lower)
-        !! Lower-case conversion for ASCII characters
-        character(len=1), intent(in) :: ch
-        character(len=1) :: lower
-        integer :: code
-
-        lower = ch
-        code = iachar(ch)
-        if (code >= iachar('A') .and. code <= iachar('Z')) then
-            lower = achar(code + 32)
-        end if
-    end function to_lower_char
-
-    logical function parse_debug_env(value)
-        !! Interpret an environment variable as boolean
-        character(len=*), intent(in) :: value
-        character(len=:), allocatable :: trimmed
-        integer :: i
-
-        trimmed = trim(adjustl(value))
-
-        if (len(trimmed) == 0) then
-            parse_debug_env = .true.
-            return
-        end if
-
-        do i = 1, len(trimmed)
-            trimmed(i:i) = to_lower_char(trimmed(i:i))
-        end do
-
-        select case (trimmed)
-        case ('0', 'false', 'off', 'no')
-            parse_debug_env = .false.
-        case default
-            parse_debug_env = .true.
-        end select
-    end function parse_debug_env
-
-    subroutine ensure_zlib_debug()
-        !! Lazy initialization for instrumentation flag
-        character(len=32) :: env_value
-        integer :: status
-
-        if (zlib_debug_initialized) return
-
-        call get_environment_variable('FORTPLOT_ZLIB_DEBUG', env_value, status=status)
-        if (status == 0) then
-            zlib_debug_enabled_state = parse_debug_env(env_value)
-        else
-            zlib_debug_enabled_state = .false.
-        end if
-        zlib_debug_initialized = .true.
-    end subroutine ensure_zlib_debug
-
-    logical function is_zlib_debug_enabled()
-        !! Query instrumentation flag
-        call ensure_zlib_debug()
-        is_zlib_debug_enabled = zlib_debug_enabled_state
-    end function is_zlib_debug_enabled
-
-    subroutine log_zlib_debug(message)
-        !! Emit debug message when instrumentation enabled
-        character(len=*), intent(in) :: message
-        if (is_zlib_debug_enabled()) then
-            print '(a)', '[fortplot:zlib] '//trim(message)
-        end if
-    end subroutine log_zlib_debug
 

Modify the parse_debug_env function to interpret an empty environment variable
value as false instead of true to follow conventional behavior.

src/external/fortplot_zlib_core.f90 [1000-1023]

 logical function parse_debug_env(value)
     !! Interpret an environment variable as boolean
     character(len=*), intent(in) :: value
     character(len=:), allocatable :: trimmed
     integer :: i
 
     trimmed = trim(adjustl(value))
 
     if (len(trimmed) == 0) then
-        parse_debug_env = .true.
+        parse_debug_env = .false.
         return
     end if
 
     do i = 1, len(trimmed)
         trimmed(i:i) = to_lower_char(trimmed(i:i))
     end do
 
     select case (trimmed)
     case ('0', 'false', 'off', 'no')
         parse_debug_env = .false.
     case default
         parse_debug_env = .true.
     end select
 end function parse_debug_env

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies counter-intuitive behavior where an empty environment variable enables debugging, and proposes a logical fix to align with standard conventions.

Medium
  • Update

@krystophny krystophny force-pushed the debug/sigbus-instrumentation branch from fa12f5a to 946f79d Compare October 3, 2025 15:03
- Created fortplot_debug_utils.f90 module for reusable debug infrastructure
- Moved debug utility functions from fortplot_zlib_core.f90
- Updated zlib_compress_into to use new debug module
- Reduced fortplot_zlib_core.f90 from 1054 to 986 lines (under 1000 line hard limit)
- File size compliance now passes: 0 critical violations

Fixes CI failure where verify-size-compliance step was blocking PR merge.
Debug functionality preserved with FORTPLOT_ZLIB_DEBUG environment variable.
Addresses qodo-merge code review feedback:
- Removed duplicate fortplot_debug_utils.f90 module
- Use existing fortplot_logging module's log_debug() function
- Debug output controlled via FORTPLOT_ZLIB_DEBUG env var + log level
- No duplication of logging infrastructure
- File size: 999 lines (still under 1000 hard limit)

Fixes:
- Debug info leakage concern (uses proper logging facility)
- Code duplication (removed redundant debug utilities)
- Maintains FORTPLOT_ZLIB_DEBUG environment variable support
@krystophny krystophny merged commit ec82037 into main Oct 4, 2025
2 checks passed
@krystophny krystophny deleted the debug/sigbus-instrumentation branch October 4, 2025 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant