search

approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
316 stars 51 forks source link

utilities: handle exit code of copy command #94

Closed p-podsiadly closed 4 years ago

p-podsiadly commented 4 years ago

Description

When building another project including ApprovalTests as a git submodule, I've got the following warnings:

warning: ignoring return value of ‘int system(const char*)’, declared with attribute warn_unused_result [-Wunused-result]

caused by calls to system() (in ClipboardReporter.h, FileUtilsSystemSpecific.h and SystemLauncher.h).

The solution

This commit replaces unchecked calls to system() with calls to a new method SystemUtils::runSystemCommand(), which throws std::runtime_error if the exit code is not 0.

Apart from suppressing the warning, this should help to locate a problem, if any of those commands actually fail.

claremacrae commented 4 years ago

Thank very much for this.

I will try this out tomorrow. I need to think through the cases. As far as I recall, in each of these cases, at the time we deliberately ignored the result of the system call - so I would like to think it through, to think whether the exception would be a problem.

On reflection, I definitely agree that the ClipboardReporter case is worth throwing.

There are lots of other calls to system that remain, and which definitely should not throw. So I think it would be better for runSystemCommand() to have a more specific name, to avoid the temptation for someone to use it in more places in future.

Perhaps runSystemCommandOrThrow()?

claremacrae commented 4 years ago

Hello @p-podsiadly,

I will try this out tomorrow. I need to think through the cases. As far as I recall, in each of these cases, at the time we deliberately ignored the result of the system call - so I would like to think it through, to think whether the exception would be a problem.

I've just reviewed them, and they are all clearly worth checking, so I've accepted the PR.

There are lots of other calls to system that remain, and which definitely should not throw. So I think it would be better for runSystemCommand() to have a more specific name, to avoid the temptation for someone to use it in more places in future.

Perhaps runSystemCommandOrThrow()?

Don't worry about this - I'll do it...

Many thanks for this - it's a definite improvement!