| Message ID | 20210823150520.25614-2-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | TST_EXP_*() macros fixes | expand |
On Mon, Aug 23, 2021 at 11:05 PM Cyril Hrubis <chrubis@suse.cz> wrote: > We have to erase the last comma because otherwise we pass down one more > empty parameter to the TST_EXP_*_() macros which then causes the FMT > string to be empty and we end up with an empty default message. > Patchset looks good from the code layer though it is a bit complicate in 2/3 stringify handling, but that should be acceptable. My only hesitating is about ##__VA_ARGS__, because it says that is still as GNU's extension but have not got into standard C. So I especially run with GitHub CI and got pretty compiling results, then I have a positive attitude on applying these patches unless there is someone who blames it broken something in a general (but I guess the possibility is very little). https://github.com/wangli5665/ltp/runs/3408080506 Feel free to add my reviews: Reviewed-by: Li Wang <liwang@redhat.com>
Hi! > Patchset looks good from the code layer though it is a bit complicate > in 2/3 stringify handling, but that should be acceptable. > > My only hesitating is about ##__VA_ARGS__, because it says that is still > as GNU's extension but have not got into standard C. Note that we have been using it in the header from the start. There were just a few places where it was missing, mostly in the variants that have been added later.
On Tue, Aug 24, 2021 at 4:48 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Patchset looks good from the code layer though it is a bit complicate > > in 2/3 stringify handling, but that should be acceptable. > > > > My only hesitating is about ##__VA_ARGS__, because it says that is still > > as GNU's extension but have not got into standard C. > > Note that we have been using it in the header from the start. There were > just a few places where it was missing, mostly in the variants that have > been added later. > Ah great, I was neglect that point. Hence it should be safe enough.
Hi Cyril, Li, to whole patchset Reviewed-by: Petr Vorel <pvorel@suse.cz> > So I especially run with GitHub CI and got pretty compiling results, then I > have > a positive attitude on applying these patches unless there is someone who > blames it broken something in a general (but I guess the possibility is > very little). > https://github.com/wangli5665/ltp/runs/3408080506 Thanks! I've also tested it in github to catch things early. Kind regards, Petr
Hello, Li Wang <liwang@redhat.com> writes: > On Tue, Aug 24, 2021 at 4:48 PM Cyril Hrubis <chrubis@suse.cz> wrote: > >> Hi! >> > Patchset looks good from the code layer though it is a bit complicate >> > in 2/3 stringify handling, but that should be acceptable. >> > >> > My only hesitating is about ##__VA_ARGS__, because it says that is still >> > as GNU's extension but have not got into standard C. >> >> Note that we have been using it in the header from the start. There were >> just a few places where it was missing, mostly in the variants that have >> been added later. >> > > Ah great, I was neglect that point. Hence it should be safe enough. > > -- > Regards, > Li Wang There is also __VA_OPT__(,) which can't be confused with concatenation. I'm not sure we should use it thought, it seems like it was only recently added to other compilers. Still might be a good idea to link to the following page somewhere: https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html#Variadic-Macros LGTM otherwise!
Hi! Thanks for the reviews, pushed.
diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h index d9d7f930f..ecc612b0d 100644 --- a/include/tst_test_macros.h +++ b/include/tst_test_macros.h @@ -70,7 +70,7 @@ extern void *TST_RET_PTR; #define TST_EXP_POSITIVE(SCALL, ...) \ do { \ - TST_EXP_POSITIVE_(SCALL, __VA_ARGS__); \ + TST_EXP_POSITIVE_(SCALL, ##__VA_ARGS__); \ \ if (TST_PASS) { \ TST_MSGP_(TPASS, " returned %ld", \ @@ -78,22 +78,22 @@ extern void *TST_RET_PTR; } \ } while (0) -#define TST_EXP_FD_SILENT(SCALL, ...) TST_EXP_POSITIVE_(SCALL, __VA_ARGS__) +#define TST_EXP_FD_SILENT(SCALL, ...) TST_EXP_POSITIVE_(SCALL, ##__VA_ARGS__) #define TST_EXP_FD(SCALL, ...) \ do { \ - TST_EXP_FD_SILENT(SCALL, __VA_ARGS__); \ + TST_EXP_FD_SILENT(SCALL, ##__VA_ARGS__); \ \ if (TST_PASS) \ TST_MSGP_(TPASS, " returned fd %ld", TST_RET, \ #SCALL, ##__VA_ARGS__); \ } while (0) -#define TST_EXP_PID_SILENT(SCALL, ...) TST_EXP_POSITIVE_(SCALL, __VA_ARGS__) +#define TST_EXP_PID_SILENT(SCALL, ...) TST_EXP_POSITIVE_(SCALL, ##__VA_ARGS__) #define TST_EXP_PID(SCALL, ...) \ do { \ - TST_EXP_PID_SILENT(SCALL, __VA_ARGS__); \ + TST_EXP_PID_SILENT(SCALL, ##__VA_ARGS__); \ \ if (TST_PASS) \ TST_MSGP_(TPASS, " returned pid %ld", TST_RET, \ @@ -124,7 +124,7 @@ extern void *TST_RET_PTR; #define TST_EXP_PASS(SCALL, ...) \ do { \ - TST_EXP_PASS_SILENT(SCALL, __VA_ARGS__); \ + TST_EXP_PASS_SILENT(SCALL, ##__VA_ARGS__); \ \ if (TST_PASS) \ TST_MSG_(TPASS, " passed", #SCALL, ##__VA_ARGS__); \ @@ -158,8 +158,8 @@ extern void *TST_RET_PTR; } \ } while (0) -#define TST_EXP_FAIL(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET == 0, SCALL, ERRNO, __VA_ARGS__) +#define TST_EXP_FAIL(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET == 0, SCALL, ERRNO, ##__VA_ARGS__) -#define TST_EXP_FAIL2(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET >= 0, SCALL, ERRNO, __VA_ARGS__) +#define TST_EXP_FAIL2(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET >= 0, SCALL, ERRNO, ##__VA_ARGS__) #endif /* TST_TEST_MACROS_H__ */
We have to erase the last comma because otherwise we pass down one more empty parameter to the TST_EXP_*_() macros which then causes the FMT string to be empty and we end up with an empty default message. Consider for example: TST_EXP_FD(open(fname, O_RDONLY)); Before the patch it would produce: foo.c:12: TPASS: returned fd 4 After it would produce: foo.c:12: TPASS: open(fname, O_RDONLY) returned fd 4 Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- include/tst_test_macros.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)