jrgemignani commented on PR #2286: URL: https://github.com/apache/age/pull/2286#issuecomment-3724736967
@jpabbuehl Thank you for your work so far! I do, however, have a few requests due to Copilot and reviewing the code changes. I'm not sure if you can see Copilot's remarks above? But, if you could review them, especially the one about sed and fragility and the PR description - > The sed replacement pattern uses a complex regular expression that may not be portable across all sed implementations. Specifically, the character class [[:space:]] is a POSIX extension that may not work with all versions of sed (though it's widely supported). > ... > The PR description states "Add conditional compilation in graphid.h" to use pass-by-value on 64-bit and pass-by-reference on 32-bit systems. However, the actual code changes do not include any modifications to graphid.h or any other C header/source files. > ... I just want your thoughts on them. As for the code, I would feel more comfortable if the code changes didn't auto-detect - ``` # 32-bit platform support: detect SIZEOF_DATUM (override with make SIZEOF_DATUM=4) # Only attempt auto-detection if SIZEOF_DATUM was not provided on the command line. ifeq ($(origin SIZEOF_DATUM), undefined) ``` I would prefer that the parameter `SIZEOF_DATUM` be passed **and** if not found, nothing happens. This way it is more expected. Is that okay with you? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
