Repository navigation
fix(plugin, preprocessor): avoid empty variadic macro arguments - #954
Open
mohdTahaRafi wants to merge 2 commits into
Open
mohdTahaRafi wants to merge 2 commits into
mohdTahaRafi wants to merge 2 commits into
Conversation
Clang warns (-Wgnu-zero-variadic-macro-arguments) when a variadic macro gets nothing for its "...". cli_core_plugin.cpp had 12 such warnings. Changes: - EXTENSION_FUNCTION and EXTENSION_FUNCTION_CHECK now take all their arguments through "..." and pick the IMPL or IMPL_VOID variant with PREPROCESSOR_ARGS_COMMA, so calls without parameter types no longer leave it empty. Call sites are unchanged. - PREPROCESSOR_FOR and PREPROCESSOR_FOR_EACH now pass the dummy ~ to IMPL_1, as IMPL_2 already does, so one-element calls (and the unused branch of zero-element calls) no longer leave it empty. Changed in preprocessor_for_body.h.in, and preprocessor_for.h is regenerated. - Add for_one and foreach_one to preprocessor_test, since a single element was not tested. With Clang 18 the warnings go from 12 to 0 in cli_core_plugin.cpp and from 9 to 0 in the sandbox plugins, and the preprocessed output is the same as before with both Clang and GCC. Fixes metacall#560
Member
|
I'm gonna run the CI, we can see the build output there. |
Member
|
If it works, next step will be adding -Werror |
Author
|
I checked the failed jobs in the CI run and many of them also happen in develop. Most failures appear to be environment related, such as package server connectivity and TSan's unexpected memory mapping error. The FreeBSD Clang logs also show no warnings from the plugin and preprocessor files modified in this PR. Regarding -Werror, I noticed four existing warnings in preprocessor_test.cpp and some -Wformat warnings in other files. I think a separate issue and a separate PR for that would be good but if you want it be resolved in this PR only then its fine. |
This was referenced Nov 12, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the Clang
-Wgnu-zero-variadic-macro-argumentswarnings from #560. Before C++20, Clang (with-Wpedantic, which MetaCall uses) gives this warning when a macro with...gets no argument for it. With Clang 18,cli_core_plugin.cpphad 12 of them, from 2 causes:EXTENSION_FUNCTION(METACALL_INT, help)andEXTENSION_FUNCTION_CHECK(HELP_ERROR), left the macro's...empty.PREPROCESSOR_FORcalledIMPL_1without the dummy~thatIMPL_2already adds, soIMPL_1's...was empty with one type, and also with zero types (the unused branch ofPREPROCESSOR_IFstill gets expanded).Changes:
plugin_interface.hpp:EXTENSION_FUNCTIONandEXTENSION_FUNCTION_CHECKnow take all their arguments through..., so it is never empty. They usePREPROCESSOR_ARGS_COMMAto check if there are argument types, and pick theIMPLorIMPL_VOIDversion (the dispatch idea from the issue). The call goes throughPREPROCESSOR_TUPLE_EXPAND_MACRO, so MSVC's old preprocessor splits the arguments too (the sameNAME (args)pattern its versions ofPREPROCESSOR_ARGS_COUNTandPREPROCESSOR_FORalready use; not tested locally). No call sites changed.preprocessor_for_body.h.inand the regeneratedpreprocessor_for.h: added the missing~inPREPROCESSOR_FORandPREPROCESSOR_FOR_EACH.preprocessor_test.cpp: addedfor_oneandforeach_onetests, because 1 element had no test (0 and many elements did). With the old headers these 2 tests give the warning, with the fix they don't. If you don't want them in this PR, I can remove them.Testing: Clang warnings went from 12 to 0 in
cli_core_plugin.cpp, and from 9 to 0 in the sandbox plugins (cli_sandbox_plugin.cpp7,sandbox_plugin.cpp2, built withOPTION_BUILD_PLUGINS_SANDBOX=ON). GCC has no new warnings. The preprocessed (-E -P) output of the 5 files that use these macros is byte-identical to before, so the generated code doesn't change. Full Clang and GCC builds pass, and the fullctestpasses 75/75 with both compilers (preprocessor-testnow has 23 cases).Screenshots (Clang 18, same flags as the build,
-std=gnu++11 -Wpedantic):Before (develop): 21 warnings in the 3 plugin files
After (this PR): 0 warnings, the files were compiled, and all tests pass
Open questions:
forandforeachtests already pass string literals through the samePREPROCESSOR_ARGS_COMMAon MSVC, so I expect it to work.preprocessor_test.cppstill has 4 of these warnings (it had 6) because it uses empty arguments on purpose. Should I change those tests?This is my first commit here, so the PR also has a separate commit adding me to
CONTRIBUTORS(the pre-commit hook asks for it).No new dependencies.
Fixes #560
Type of change
Checklist:
make testorctest -VV -R <test-name>)../docker-compose.sh test &> outputand attached the output. (not breaking, and my disk doesn't have space for the Docker images, so I ran the full test suite locally with Clang and GCC: 75/75)OPTION_BUILD_ADDRESS_SANITIZERor./docker-compose.sh test-address-sanitizer &> outputandOPTION_TEST_MEMORYCHECK. (ASan + UBSan: 74/74 pass, 0 reports; MetaCall skips the sandbox test under sanitizers, so 74 not 75. Memcheck: same 3 failures and same defects asdevelopwithout my change, all in Node and simdjson, none in the changed code)OPTION_BUILD_THREAD_SANITIZERor./docker-compose.sh test-thread-sanitizer &> output. (74/74 pass, 0 reports)Helgrindin case my code works with threading. (no threading in the change; ran the CLI under Helgrind, all reports come from Node, same as ondevelop)cmake --build build --target clang-format(orcmake -DMODE=fix -P cmake/Format.cmake) in order to format my code and my code follows the style guidelines.AI-ASSISTED: yes/no, model, human reviewer) and I own each line.AI-ASSISTED: yes, Claude Opus 5.5, Reviewed and tested by me.