fix: use quoteShell #33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to replace manual shell escaping with Nim's quoteShell function for defining a C preprocessor macro. The change affects two files where a C preprocessor definition is passed to the compiler via the passc pragma.
Key Changes:
- Replaces
\\\"manual escaping withquoteShell("\"lsquic_xxhash.h\"")in bothprelude.nimandlsquic/lsquic_ffi.nim
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| prelude.nim | Modified C preprocessor flag definition to use quoteShell instead of manual escaping |
| lsquic/lsquic_ffi.nim | Modified C preprocessor flag definition to use quoteShell instead of manual escaping |
Critical Issue Found: The usage of quoteShell in this PR is incorrect. The function is being passed a string that already contains quote characters ("\"lsquic_xxhash.h\""), which will cause quoteShell to add additional escaping/quoting around these existing quotes. This is not the intended behavior for defining a C preprocessor macro. The original approach with manual escaping (\\\") was correct for this use case, as it ensures the C preprocessor receives the macro definition with proper string literal quotes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
prelude.nim
Outdated
| type ptrdiff_t* {.importc: "ptrdiff_t", header: "<stddef.h>".} = int | ||
|
|
||
| {.passc: "-DXXH_HEADER_NAME=\\\"lsquic_xxhash.h\\\"".} | ||
| {.passc: "-DXXH_HEADER_NAME=" & quoteShell("\"lsquic_xxhash.h\"").} |
There was a problem hiding this comment.
The usage of quoteShell appears incorrect here. quoteShell("\"lsquic_xxhash.h\"") passes a string containing quote characters to quoteShell, which will add additional escaping/quoting around it for shell safety.
The goal is to define a C preprocessor macro XXH_HEADER_NAME with the value "lsquic_xxhash.h" (as a string literal). The correct approach would be:
{.passc: "-DXXH_HEADER_NAME=" & quoteShell("lsquic_xxhash.h").}However, this still may not produce the desired C string literal. The original approach with manual escaping (\\\") was actually correct for passing string literals to C preprocessor definitions. If the goal is to use quoteShell for cross-platform safety, the string literal quotes should be added after quoting:
{.passc: "-DXXH_HEADER_NAME=\\\"" & quoteShell("lsquic_xxhash.h") & "\\\"".}But this would double-escape. The simplest correct fix using quoteShell would be to not include the quotes in the input.
| {.passc: "-DXXH_HEADER_NAME=" & quoteShell("\"lsquic_xxhash.h\"").} | |
| {.passc: "-DXXH_HEADER_NAME=\\\"lsquic_xxhash.h\\\"".} |
lsquic/lsquic_ffi.nim
Outdated
|
|
||
|
|
||
| {.passc: "-DXXH_HEADER_NAME=\\\"lsquic_xxhash.h\\\"".} | ||
| {.passc: "-DXXH_HEADER_NAME=" & quoteShell("\"lsquic_xxhash.h\"").} |
There was a problem hiding this comment.
The usage of quoteShell appears incorrect here. quoteShell("\"lsquic_xxhash.h\"") passes a string containing quote characters to quoteShell, which will add additional escaping/quoting around it for shell safety.
The goal is to define a C preprocessor macro XXH_HEADER_NAME with the value "lsquic_xxhash.h" (as a string literal). The correct approach would be:
{.passc: "-DXXH_HEADER_NAME=" & quoteShell("lsquic_xxhash.h").}However, this still may not produce the desired C string literal. The original approach with manual escaping (\\\") was actually correct for passing string literals to C preprocessor definitions. If the goal is to use quoteShell for cross-platform safety, the string literal quotes should be added after quoting:
{.passc: "-DXXH_HEADER_NAME=\\\"" & quoteShell("lsquic_xxhash.h") & "\\\"".}But this would double-escape. The simplest correct fix using quoteShell would be to not include the quotes in the input.
Co-authored-by: Esteban C Borsani <[email protected]>
richard-ramos
force-pushed
the
fix/use-quoteshell
branch
from
91f0fc3 to
a3e88c5
Compare
|
@nitely it seems windows did not like the '<lsquic_xxhash.h>' :( |
I checked it got passed the include error when compiling Nimbus (it failed later to find some boringSSL/OpenSSL files I think). The single quotes should work in mingw. |
|
🤔 i wonder then why it failed here. ... there's some diff in both environments that i am missing (besides nimbus using nimbus-build-system and nim-lsquic using nimble) |
The only difference I can see is Nimbus using bash as shell, and this repo uses msys2, and Nimbus runs everything with mingw make. |
No description provided.