Re: Q_ASSERT(!FalseSecurity)

2018-03-12 Thread Michael Heidelbach

Got it.
Thanks to you all,
Michael

On 10.03.2018 10:53, Albert Astals Cid wrote:

El dissabte, 10 de març de 2018, a les 10:37:12 CET, Sven Brauch va escriure:

Hi,

On 03/10/2018 09:53 AM, Michael Heidelbach wrote:

Am I getting something wrong? Or is

"Q_ASSERT(m_writeTrans);

m_writeTrans->commit();"

providing false security?

a lot of KDE code is written this way. It will end up crashing in both
debug and release, but in debug the message will be clearer. I think
this makes sense for cases where you think it is very unlikely that the
condition will not be met. I also think adding the conditional
everywhere often makes debugging harder because it prevents the crash.

That's the important part, sure, adding an if will make it not crash, but most
probably it won't do what you wanted either.

So instead of a clear crash that you can get reported and hopefully fixed, you
get the app misbehaving but it's not so obvious to the user so you may not get
a report and it may never be fixed.

Cheers,
   Albert


Best,
Sven








Re: Q_ASSERT(!FalseSecurity)

2018-03-11 Thread Thiago Macieira
On Saturday, 10 March 2018 00:53:24 PDT Michael Heidelbach wrote:
> Hi!
> 
> Am I getting something wrong? Or is
> 
> "Q_ASSERT(m_writeTrans);
> 
> m_writeTrans->commit();"
> 
> providing false security?

This is not false security. It's not security, period.

You use a Q_ASSERT when a violation indicates a bug that needs to be fixed, 
instead of a normal condition. So the above code is fine.

> Shouldn't it better be
> 
> "Q_ASSERT(m_writeTrans);
> 
> if (m_writeTrans) {
> 
>  m_writeTrans->commit(); ?

It should definitely NOT be this.

Use either Q_ASSERT or the if, not both.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center





Re: Q_ASSERT(!FalseSecurity)

2018-03-10 Thread Albert Astals Cid
El dissabte, 10 de març de 2018, a les 10:37:12 CET, Sven Brauch va escriure:
> Hi,
> 
> On 03/10/2018 09:53 AM, Michael Heidelbach wrote:
> > Am I getting something wrong? Or is
> > 
> >"Q_ASSERT(m_writeTrans);
> > 
> >m_writeTrans->commit();"
> > 
> > providing false security?
> 
> a lot of KDE code is written this way. It will end up crashing in both
> debug and release, but in debug the message will be clearer. I think
> this makes sense for cases where you think it is very unlikely that the
> condition will not be met. I also think adding the conditional
> everywhere often makes debugging harder because it prevents the crash.

That's the important part, sure, adding an if will make it not crash, but most 
probably it won't do what you wanted either.

So instead of a clear crash that you can get reported and hopefully fixed, you 
get the app misbehaving but it's not so obvious to the user so you may not get 
a report and it may never be fixed.

Cheers,
  Albert

> 
> Best,
> Sven






Re: Q_ASSERT(!FalseSecurity)

2018-03-10 Thread Sven Brauch
Hi,

On 03/10/2018 09:53 AM, Michael Heidelbach wrote:
> Am I getting something wrong? Or is
> 
>    "Q_ASSERT(m_writeTrans);
> 
>    m_writeTrans->commit();"
> 
> providing false security?

a lot of KDE code is written this way. It will end up crashing in both
debug and release, but in debug the message will be clearer. I think
this makes sense for cases where you think it is very unlikely that the
condition will not be met. I also think adding the conditional
everywhere often makes debugging harder because it prevents the crash.

Best,
Sven



signature.asc
Description: OpenPGP digital signature


Re: Q_ASSERT(!FalseSecurity)

2018-03-10 Thread James Augustus Zuccon
Good pickup - I'm not sure of the answer but my interpretation of it is the
same as yours.

Here's a StackOverflow thread that might be good reference for anyone else
that wants to chime in:
https://stackoverflow.com/questions/12573230/q-assert-release-build-semantics#12573446

Cheers!

On Sat, Mar 10, 2018 at 7:53 PM, Michael Heidelbach 
wrote:

> Hi!
>
> Am I getting something wrong? Or is
>
> "Q_ASSERT(m_writeTrans);
>
> m_writeTrans->commit();"
>
> providing false security?
>
> Shouldn't it better be
>
> "Q_ASSERT(m_writeTrans);
>
> if (m_writeTrans) {
>
> m_writeTrans->commit();  ?
>
> Baloo's code is full of Q_ASSERTs that have no corresponding if-guard.
>
> To my understanding those would slip through in release-builds. Am I wrong?
>
>
> Please enlighten me,
>
> Michael
>
>
>
>
>
>