Skip to content

Conversation

@congyong
Copy link

@congyong congyong commented Oct 27, 2025

Summary

Operating System: Windows 11
Language: Verilog
Linter Tool: Verilator 4.216 2021-12-05 rev v4.216-22-g141c5da3 & 5.026
Tested Code: a.zip


Motivation and Context

While testing the Verilator integration on Windows 11, I observed that the temporary file path generated by s:TemporaryFilename was inconsistent with the expected Windows path format.
For example:

D:/Personal/Temp/V6UA83E.tmp\a.v

The existing implementation uses the following logic ( function! s:TemporaryFilename(buffer) in verilator.vim) :

return ale#util#Tempname() . (has('win32') ? '\' : '/') . l:filename

This approach always appends a backslash (\) on Windows systems.
However, on Windows 11, temporary paths returned by ale#util#Tempname() may contain either forward slashes (/) or backslashes (\), resulting in mixed and invalid paths.


Description of Changes in ‘command.vim’

To ensure consistent and valid path construction across Windows versions while maintaining backward compatibility,
the separator is now determined dynamically based on the actual temporary path:

if has('win32') && l:tmpname =~ '\\'
    let l:sep = '\'
else
    let l:sep = '/'
endif

This change ensures correct handling of paths on Windows 11 and remains compatible with older Windows systems.


Changes in verilator.vim

Windows paths differ from Unix-style paths in that they include drive letters (e.g., D:) and additional colons.
The parsing logic in verilator.vim has been updated to correctly handle such cases, preventing path-related errors when running Verilator on Windows.

if has('win32')
    " match filename until last colon before line number
    let l:pattern = '^%\(Warning\|Error\)[^:]*:\s*\(.\{-}\):\(\d\+\):\(\d\+\)\?:\? \(.\+\)$'
else
    let l:pattern = '^%\(Warning\|Error\)[^:]*:\s*\([^:]\+\):\(\d\+\):\(\d\+\)\?:\? \(.\+\)$'
endif

Testing

The changes were tested locally on Windows 11 with Verilator.
After applying the modifications, the Verilog linter executed successfully, and temporary files were created using valid path formats.


Additional Notes

These updates improve cross-platform consistency without affecting Unix-based environments.
No regressions were observed in local testing.

@rymdbar
Copy link
Contributor

rymdbar commented Oct 27, 2025

Thanks for your neat and well-explained pull request, @congyong!

One worry I see is that the replaced pattern is still used elsewhere within the ale sources. Running grep -R "has('win32') ? '.' : '/'" . | wc -l shows 20 matches.

Fixing this instance is great. Fixing the others too would be even better. Not every occurrence of the pattern might be lucky enough to have access to a value from a freshly called tempname() to determine separator from. Thus as an alternative solution I suggest adding an ale#util#PathSeparator() returning either '/' or '\'. Likely using the same logic as tempname() documents and using that new utility function instead.

Maybe something like this? (Not verified to work)

function! ale#util#Pathseparator() abort
    let l:separator = ''

    if has('win32') && exists('+shellslash')
        if &shellslash
            let l:separator = '/'
        else
            let l:separator = '\\'
        endif
    elseif has('win32')
        if &shellcmdflag =~ '^-' && &shell !~ 'powershell\|pwsh'
            let l:separator = '/'
        else
            let l:separator = '\\'
        endif
    else
        let l:separator = '/'
    endif

    return l:separator
endfunction

Checking whether the result of tempname() matches '\' or '/' depending on that suggested …#PathSeparator() function returns would make a great (couple of) test case(s).

Would you agree addressing the pattern seems like a reasonable thing to do? Could you be up for doing it within the scope of this pull request?

@congyong
Copy link
Author

congyong commented Oct 27, 2025

Hello, rymdbar
initially, I also intended to approach it this way. However, I was concerned about potential historical contexts or legacy considerations that I might not have fully understood. so I only fixed this specific instance.
Going forward, I can allocate some time to review other sections of the code to assess whether they can also be repaired by replacing the relevant logic with SAME function.

and There are too many corners in the forward combing, so consider other ways to implement the code.
maybe we can try this function

" Detect the best path separator for the current environment.
function! ale#util#PathSeparator(...) abort
    " 1. Get base dir from args OR  a temporary file name (reflects the real system path style)
    let l:tmpname = a:0 ? a:1 : ale#util#Tempname()
    " 2. On Windows, if the path contains '\', use it.
    if has('win32')
        if l:tmpname =~# '\\'
            return '\\'
        endif
        " Otherwise, fallback to '/' (e.g., WSL, Git Bash, MSYS2)
        return '/'
    endif
    " 3. On Unix-like systems, always use '/'
    return '/'
endfunction

~Yong

@congyong
Copy link
Author

congyong commented Oct 28, 2025

Background of this function: I have reviewed the relevant code that uses Win32 for checking, and most of its calling scenarios contain directory information. Therefore, I added a judgment logic to the PathSep function. If the passed-in args include a directory, the format of this directory will be verified; if not, a temporary file (Tempfile) will be generated to parse the directory format.
There are many failed test cases (case failed). I need to check this issue when free , PAUSE

In addition, the code style under ale_linter is not consistent, and each file needs to be reviewed one by one.

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.

2 participants