Skip to content

Conversation

@Malmahrouqi3
Copy link
Contributor

@Malmahrouqi3 Malmahrouqi3 commented Jul 8, 2025

User description

Description

Concerning(#928),
Quick check for old raw directives.


PR Type

Enhancement


Description

  • Add CI check for raw !$acc or !$omp directives

  • Enforce macro paradigm usage in source code

  • Exclude specific macro files from directive check


Changes diagram

flowchart LR
  A["Source Code"] --> B["CI Lint Check"]
  B --> C["Scan for Raw Directives"]
  C --> D["Exclude Macro Files"]
  D --> E["Fail if Found"]
Loading

Changes walkthrough 📝

Relevant files
Continuous-integration
lint-source.yml
Add raw directive detection to CI                                               

.github/workflows/lint-source.yml

  • Add new CI step to check for raw !$acc or !$omp directives
  • Use grep with exclusions for parallel_macros.fpp and syscheck.fpp
  • Fail build if any raw directives are found in source code
  • +4/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copilot AI review requested due to automatic review settings July 8, 2025 17:21
    @Malmahrouqi3 Malmahrouqi3 requested a review from sbryngelson as a code owner July 8, 2025 17:21
    @qodo-code-review
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    928 - Fully compliant

    Compliant requirements:

    • Add CI check for raw !$acc or !$omp directives
    • Fail if any raw directives are found in source code
    • Exclude macro.fpp type files from the check
    • Enforce macro paradigm usage instead of raw directives

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Regex Pattern

    The grep pattern uses !\$acc\|!\$omp which may not match all variations of OpenMP/OpenACC directives. Consider if patterns like !$ACC, !$OMP, or with different spacing should also be caught.

    ! grep -iR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="syscheck.fpp" ./src/*
    
    File Exclusions

    Only excludes parallel_macros.fpp and syscheck.fpp but ticket mentions excluding macro.fpp type files. Verify if other macro files should be excluded or if the current exclusions are sufficient.

    ! grep -iR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="syscheck.fpp" ./src/*
    
    Copy link
    Contributor

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR adds a new CI lint step to catch any raw OpenACC/OpenMP directives left in the source tree.

    • Introduce a GitHub Actions step named “Looking for raw directives” that greps for !$acc or !$omp in ./src
    • Exclude known macro files (parallel_macros.fpp, syscheck.fpp)
    Comments suppressed due to low confidence (2)

    .github/workflows/lint-source.yml:39

    • [nitpick] Limit the scan to Fortran source extensions to avoid false positives in non-code files. For example, add --include="*.f90" --include="*.fpp" after the -iR flag.
            ! grep -iR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="syscheck.fpp" ./src/*
    

    .github/workflows/lint-source.yml:39

    • [nitpick] Exclude binary files to prevent grep errors on non-text files by adding -I (ignore binary) to the command, e.g., grep -iR -I.
            ! grep -iR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="syscheck.fpp" ./src/*
    
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jul 8, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @sbryngelson sbryngelson merged commit 6cb2376 into MFlowCode:master Jul 15, 2025
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    2 participants