Conversation
- VectorRef&OpaqueRef m_kind = CV_UNKNOWN - added same-type overload for saturate() - sanitized resize value in ByteMemoryInStream::operator>> (std::string& str) - handled throws from ~GStreamingExecutor()
| static inline DST saturate(SRC x) | ||
| { | ||
| // only integral types please! | ||
| GAPI_DbgAssert(std::is_integral<DST>::value && |
There was a problem hiding this comment.
can it be as static_assert ?
There was a problem hiding this comment.
Answering question, no, it failed when I tried.
But I decided that, as I've already been using enable_if semantics, this condition should be there, too
Also, there were warnings on Windows compilers about the next saturate function overload, so I rewrote it with enable_ifs, too
| str.clear(); | ||
| } else { | ||
| str.resize(sz); | ||
| str.resize(std::size_t(sz)); |
There was a problem hiding this comment.
static_cast by code style(?)
There was a problem hiding this comment.
Yes, I think you're right. Just really don't like to write it.
Maybe there are no such strict conventions in OCV, but you're still right :)
There was a problem hiding this comment.
it is the place where other reviewers will pay attention, personally i dislike such writing long-symbol cast too, but it's still strong recommendation
| GAPI_LOG_WARNING(NULL, message.str()); | ||
| } | ||
| } else { | ||
| destruct(); |
There was a problem hiding this comment.
destruct throws error?
I checked code and found root cause of exception
// Pull messages from the final queue to ensure completion
Cmd cmd;
while (!cv::util::holds_alternative<Stop>(cmd))
{
m_out_queue.pop(cmd);
}
GAPI_Assert(cv::util::holds_alternative<Stop>(cmd)); <----
And
- Assert looks redundant, because while-loop won't breaks otherwise
- this may be solved by more simplest way then using std::uncaught_exception
a) separate stop() on stop() & wait_stop/shutdown(), and just call stop() in ~dtor because it looks like you should not extracts some cmd (because currently we're dropping them anyway) OR trying to wait it several cycles, then big WARNING printed
b) do not throw Assert exception at all
From my perspective std::uncaught_exception is redundant and stirred ups and should be used in more dead-ended situations. But stop should not throw anything
There was a problem hiding this comment.
I think your comment is highly reasonable, firstly about freeing destruction from exceptions: I'd rather dive a bit deeper & solve as many rootcauses as possible than simply cover all with try-catch.
Secondly, I like this stop() separation suggestion.
There was a problem hiding this comment.
Though, there are much more places where destructor can fall over.
The most obvious is assert in push() of concurrent_bounded_queue:
if (m_capacity && m_capacity == m_data.size()) {
// if there is a limit and it is reached, wait
m_cond_full.wait(lock, [&](){return m_capacity > m_data.size();});
GAPI_Assert(m_capacity > m_data.size());
}
and maybe some others.
@dmatveev what do you think on this destructor issue?
There was a problem hiding this comment.
@OrestChura i see you point, but: from my perspective bounded_queue should not throws too because std::conditional_variable here retain its postcondition after .wait - so we do not need to debug standard c++ library and this check looks impossible according to bounded_queue design. So this throwing exception reason MUST be predictable, because this code doesn't interact with some user input or depends some OS/IO/RAM primitives setting but all behavior ensured by STD. I mean that it cannot throw sporadically and 100% determined from current code logic from 100% determined cases.
SO, there are should have been unit test for that "case" which ensure that exception must or must not to ensure guarantee that bounded_queue is consistent at all. But I'm pretty sure that it is useless here
What's about other exception: std::bad_alloc and so on - they are uncontrollable/unpredictable from us usually and/or based on user input or other system settings and SHOULD be checked in appropriate places OR ignored and falls into destructor and let program terminate in usual way, If it doesn't contradict with current policy
Example of predictable & controllable behavior:
try {
cin >> size; // user input `-1`
char* new char [size]; // usually throws std::bad_alloc on `-1`
} catch (...)
{
cerr << "Too much size requested: " << size << std::endl;
}
Example of non predictable behavior
std::string aaa = "aaa";
foo(aaa);
foo (std::string bbb) <---- copy may throw
The second example usually in not handled in most common applications and should be ignored
In result i assume that coverity doesn't dive into STD code and doesn't check throwing from vector push and so on and it's limited by application code only
| explicit GStreamingExecutor(std::unique_ptr<ade::Graph> &&g_model, | ||
| const cv::GCompileArgs &comp_args); | ||
| ~GStreamingExecutor(); | ||
| ~GStreamingExecutor() noexcept(false); // throwing when a regular destruction fails only; |
There was a problem hiding this comment.
i thinks stop should not throw
|
@OrestChura about destructor: |
sivanov-work
left a comment
There was a problem hiding this comment.
Please ask @anna-khakimova to review SIMD part
| } catch (const std::logic_error& e) { | ||
| std::stringstream message; | ||
| message << "~GStreamingExecutor() threw exception with message '" << e.what() << "'\n"; | ||
| GAPI_LOG_WARNING(NULL, message.str()); |
| CV_ALWAYS_INLINE void run_convertto(DST *out, const SRC *in, const int length) | ||
| { | ||
| // manual SIMD if need rounding | ||
| GAPI_Assert(( std::is_same<SRC,float>::value )); |
There was a problem hiding this comment.
reject for double only? in this case i think we can change SFINAE from std::is_floating_point<SRC>::value to is_same<float>
There was a problem hiding this comment.
For this point, I assumed that if we utilize SFINAE there will be non-informative compile error; in the case of Assert, it will be clear why and where the problem is.
So I'd prefer leaving Assert
There was a problem hiding this comment.
Any kind of error is better than no error, i agree
But when you spend ~30min to compile entire application and then got runtime error (or not got which is worse) about error which were managed to be found at compile time then fix it and spend 30 minutes again - it's disappointing.
So compile-time error should be found at compile time, I think. If you worried about nasty compilation message please consider static_assert(..., "Something horrible") here
There was a problem hiding this comment.
Makes sense. Made it static_assert, and added it in some other overloads too
|
|
||
| // compute faster if no alpha no beta | ||
| if (alpha == 1 && beta == 0) | ||
| if (1 == alpha && 0 == beta) |
There was a problem hiding this comment.
because alpja & beta are floating point - i do not think it is right to compare them with straight to integer 1 & 0 is correct across using epsilon
| try { | ||
| if (state == State::READY || state == State::RUNNING) | ||
| stop(); | ||
| } catch (const std::logic_error& e) { |
There was a problem hiding this comment.
But why only logic_error ?
…issues [G-API] Fixed Coverity issues * Fixed Coverity issues - VectorRef&OpaqueRef m_kind = CV_UNKNOWN - added same-type overload for saturate() - sanitized resize value in ByteMemoryInStream::operator>> (std::string& str) - handled throws from ~GStreamingExecutor() * Catching exception by const ref * Addressing Sergey's comments * Applied enable_if semanitcs to saturate(x, round) too * Removed uncaught_exception, made destructor noexcept back * Split Fluid ConvertTo to multiple functions to avoid ifs; added CV_ALWAYS_INLINE * Added FIXME to address throwings from stop() * Fix standalone * Addressing comments * Guarded SIMD optimizations properly * Removed excess parameter from simd_impl functions
cv::detail::OpaqueKind m_kindfield ofVectorRefandOpaqueRefis now set toCV_UNKNOWNby defaultsaturate()function calls with the same input and output typeByteMemoryInStream::operator>> (std::string& str):std::string::resize()method expectsstd::size_twhen used to getuint32_tinsteadPull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.