Skip to content

Conversation

@firewave
Copy link
Collaborator

I need to add parameters to some check() functions in the tests and things are already pretty messy with having to specify all the default values - readability aside.

I found this on https://stackoverflow.com/a/49572324/532627 - apparently the CC BY-SA license by StackOverflow allows the usage within GPL.

@firewave
Copy link
Collaborator Author

We can probably get rid of some of the redundant code with more macro magic.

This is just an example. There's some tests which can profit even more than this. This might also help with some of the issues I will face using custom settings in some cases on my way to make settings immutable in tests.

@firewave firewave force-pushed the test-dinit branch 2 times, most recently from 25b8104 to f4442ed Compare May 3, 2023 15:52
@firewave firewave marked this pull request as ready for review May 3, 2023 15:52
test/helpers.h Outdated
);
*/
#define dinit(T, ...) \
([&]{ T ${}; __VA_ARGS__; return $; }())
Copy link
Contributor

Choose a reason for hiding this comment

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

So doing dinit(S, $.i = 1, $.j = 2) wont evaluate the initialization in order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That should not be an issue in most of the cases. There are a few where that could be a problem though.

@pfultz2
Copy link
Contributor

pfultz2 commented May 3, 2023

With some more macro stuff, you could improve this more:

#define PRIMITIVE_APPLY(m, data, x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11, x12, x13, x14, x15, x16, x17, x18, x19, x20, x21, x22, x23, x24, x25, x26, x27, x28, x29, x30, x31, x32, x33, x34, x35, x36, x37, x38, x39, x40, x41, x42, x43, x44, x45, x46, x47, x48, x49, x50, x51, x52, x53, x54, x55, x56, x57, x58, x59, x60, x61, x62, ...) \
    m(x0, data) m(x1, data) m(x2, data) m(x3, data) m(x4, data) m(x5, data) m(x6, data) m(x7, data) m(x8, data) m(x9, data) m(x10, data) m(x11, data) m(x12, data) m(x13, data) m(x14, data) m(x15, data) m(x16, data) m(x17, data) m(x18, data) m(x19, data) m(x20, data) m(x21, data) m(x22, data) m(x23, data) m(x24, data) m(x25, data) m(x26, data) m(x27, data) m(x28, data) m(x29, data) m(x30, data) m(x31, data) m(x32, data) m(x33, data) m(x34, data) m(x35, data) m(x36, data) m(x37, data) m(x38, data) m(x39, data) m(x40, data) m(x41, data) m(x42, data) m(x43, data) m(x44, data) m(x45, data) m(x46, data) m(x47, data) m(x48, data) m(x49, data) m(x50, data) m(x51, data) m(x52, data) m(x53, data) m(x54, data) m(x55, data) m(x56, data) m(x57, data) m(x58, data) m(x59, data) m(x60, data) m(x61, data) m(x62, data) 

#define APPLY(m, data, ...) PRIMITIVE_APPLY(m, data, __VA_ARGS__,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,)

#define dinit_each(x, data) data x ;

#define dinit(T, ...) \
    ([&]{ T x{}; APPLY(dinit_each, x, __VA_ARGS__) return x; }())

And then initialize with dinit(S, .i = 1, .j = 2) which will match the designated initialization in C++20 and C99 that perhaps we could make it use that on compilers that support it(ie #define dinit(T, ...) T{__VA_ARGS__})

@firewave
Copy link
Collaborator Author

firewave commented May 3, 2023

Very interesting - though a bit stomach turning.

But I gotta look into the compiler errors on GCC 4.8 first though. I had to specify that strange noexcept default constructor or it would complain about the default initialization in the function declaration. That apparently doesn't fly with those pseudo-C++11 compliant compilers.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

I think dinit is nice.

@firewave firewave marked this pull request as draft May 4, 2023 08:54
@firewave
Copy link
Collaborator Author

firewave commented May 4, 2023

I will try to make it C++20 compliant first.

@chrchr-github
Copy link
Collaborator

chrchr-github commented May 4, 2023

Snarky comment: we wouldn't need any macros if we just switched to C++20 already...

@firewave
Copy link
Collaborator Author

firewave commented May 4, 2023

I decided to leave out several snarky remarks of my reply. 👺

C++20 is not fully supported by the compilers yet and even the existing parts appear to be quite buggy. And if we ever raise the C++ version I would prefer to go up incrementally to be able to track down performance, compiler time or other issues more easily. Also the used features should be upped incrementally since some of some can have some detrimental effects.

Also variadic templates are not much better in reading and writing them. And my IDE shows me the resolved macros so that would actually be a plus for macros in my case.

We should try to clean/fix up the existing things before opening new cans of worms...

@chrchr-github
Copy link
Collaborator

C++20 has designated initializers (https://en.cppreference.com/w/cpp/language/aggregate_initialization), not sure where variadic templates come into play.
Anecdotal evidence: MSVC 2019+ supports C++20 just fine. But, as mentioned, my comment wasn't entirely serious.

@firewave
Copy link
Collaborator Author

@pfultz2 The suggested C++20 compliant approach causes compiler warnings and is probably not working:

/mnt/s/GitHub/cppcheck-fw/test/testprocessexecutor.cpp:138:20: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
  138 |               "}", dinit2(CheckOptions, .showtime = SHOWTIME_MODES::SHOWTIME_SUMMARY));
      |                    ^
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:117:18: note: expanded from macro 'dinit2'
  117 |     ([&]{ T x{}; APPLY(dinit_each, x, __VA_ARGS__) return x; }())
      |                  ^
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:112:127: note: expanded from macro 'APPLY'
  112 | #define APPLY(m, data, ...) PRIMITIVE_APPLY(m, data, __VA_ARGS__,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,)
      |                                                                                                                               ^
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:109:9: note: macro 'PRIMITIVE_APPLY' defined here
  109 | #define PRIMITIVE_APPLY(m, data, x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11, x12, x13, x14, x15, x16, x17, x18, x19, x20, x21, x22, x23, x24, x25, x26, x27, x28, x29, x30, x31, x32, x33, x34, x35, x36, x37, x38, x39, x40, x41, x42, x43, x44, x45,[ 81%] Building CXX object test/CMakeFiles/testrunner.dir/testsettings.cpp.o
 x46, x47, x48, x49, x50, x51, x52, x53, x54, x55, x56, x57, x58, x59, x60, x61, x62, ...) \
      |         ^
/mnt/s/GitHub/cppcheck-fw/test/testprocessexecutor.cpp:138:20: warning: expression result unused [-Wunused-value]
  138 |               "}", dinit2(CheckOptions, .showtime = SHOWTIME_MODES::SHOWTIME_SUMMARY));
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:117:36: note: expanded from macro 'dinit2'
  117 |     ([&]{ T x{}; APPLY(dinit_each, x, __VA_ARGS__) return x; }())
      |                                    ^
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:112:48: note: expanded from macro 'APPLY'
  112 | #define APPLY(m, data, ...) PRIMITIVE_APPLY(m, data, __VA_ARGS__,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,)
      |                                                ^~~~
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:110:23: note: expanded from macro 'PRIMITIVE_APPLY'
  110 |     m(x0, data) m(x1, data) m(x2, data) m(x3, data) m(x4, data) m(x5, data) m(x6, data) m(x7, data) m(x8, data) m(x9, data) m(x10, data) m(x11, data) m(x12, data) m(x13, data) m(x14, data) m(x15, data) m(x16, data) m(x17, data) m(x18, data) m(x19, data) m(x20, data) m(x21, data) m(x22, data) m(x23, data) m(x24, data) m(x25, data) m(x26, data) m(x27, data) m(x28, data) m(x29, data) m(x30, data) m(x31, data) m(x32, data) m(x33, data) m(x34, data) m(x35, data) m(x36, data) m(x37, data) m(x38, data) m(x39, data) m(x40, data) m(x41, data) m(x42, data) m(x43, data) m(x44, data) m(x45, data) m(x46, data) m(x47, data) m(x48, data) m(x49, data) m(x50, data) m(x51, data) m(x52, data) m(x53, data) m(x54, data) m(x55, data) m(x56, data) m(x57, data) m(x58, data) m(x59, data) m(x60, data) m(x61, data) m(x62, data)
      |                       ^~~~
/mnt/s/GitHub/cppcheck-fw/test/helpers.h:114:29: note: expanded from macro 'dinit_each'
  114 | #define dinit_each(x, data) data x ;
      |                             ^~~~

Since it was already approved and making it compliant is a rather minor change afterwards I will commit this version for now. I also won't apply it broadly for now. It will be mainly for some targeted cases. This also blocks another pending change (cannot remember the specific one off the top my head).

@firewave firewave marked this pull request as ready for review August 3, 2023 08:42
@firewave firewave merged commit 5d201c4 into danmar:main Aug 4, 2023
@firewave firewave deleted the test-dinit branch August 4, 2023 11:56
pfultz2 added a commit to pfultz2/cppcheck that referenced this pull request Aug 6, 2023
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.

4 participants