[Development] QOptional

[Julien Blanc]

On 21/08/2014 13:01, Poenitz Andre wrote:
> Julien Blanc wrote:
>> There are two strong arguments in favor of optional :
>> - real life tends to show that the optional &ok parameter is often
>> missed in context it should not, leading to hard to find bugs.
>> - optional is much more friendly to static verification than &ok (dependancy between variables is a mess to check).
>>
> I am actually not quite sure what you mean with the second item.

I’m talking about what tools like coverity / polyspace can verify on the 
code. Checking that an optional is not used when it is not tested for 
nullness is doable (it is a lot like static checking against null 
pointers dereference), whereas checking for another variable is beyond 
what current static checkers can do afaik.

>
> Something like the following?
>
> struct S { int m_a, int m_b, int m_c);
>
> // "Tradtional, wrong result"
> bool S::parse(QString a, QString b, QString c)
> {
>      bool ok;
>      m_a = a.toInt(&ok);
>      m_b = b.toInt(&ok); // Oops, overwrites first ok value.
>      m_c = c.toInt(&ok);
>      return ok;
> }

I wasn’t thinking about this one, but that’s an issue, yes.

>
> vs
>
> // "Tradtional, right result"
> bool S::parse(QString a, QString b, QString c)
> {
>      bool oa, ob, oc;
>      m_a = a.toInt(&oa);
>      m_b = b.toInt(&ob);
>      m_c = c.toInt(&oc);
>      return oa && ob && oc;
> }

I would not recommend writing code like that (although i think it’s 
widespread). I would expect that parse, when failing, does not alter the 
object at all, so that it stays in a corerent state (aka 
“strong-guarantee”). This is not the case with such implementation.

I would write it that way :

    bool S::parse(QString a, QString b, QString c)
    {
         int tmpa, tmpb, tmpc;
         bool oa, ob, oc;
         tmpa = a.toInt(&oa);
         tmpb = b.toInt(&ob);
         tmpc = c.toInt(&oc);
         if(oa && ob && oc)
         {
              m_a = tmpa;
              m_b = tmpb;
              m_c = tmpc;
              return true;
         }
         return false;
     }


>
> vs
>
> // "Optional"
> bool S::parse(QString a, QString b, QString c)
> {
>      auto oa = a.toInt();
>      auto ob = b.toInt();
>      auto oc = c.toInt();
>      m_a = *oa;
>      m_b = *ob;
>      m_c = *oc;
>      return oa && ob && oc;


This one is obviously wrong and would trigger an undefined behaviour if 
any string is not convertible to int. The good thing is that I expect 
smart static verification tools to tell you this. To fix it, I would 
write it that way :

     bool S::parse(QString a, QString b, QString c)
    {
         auto oa = a.toInt();
         auto ob = b.toInt();
         auto oc = c.toInt();
         if(oa && ob && oc)
         {
              m_a = *oa;
              m_b = *ob;
              m_c = *oc;
              return true;
         }
         return false;
     }

Note that we gained strong guarantee in the process without even aiming 
for it, at no line of code cost.

>
> vs
>
> // "isInt"
> bool S::parse(QString a, QString b, QString c)
> {
>      bool oa = a.isInt(&m_a);
>      bool ob = b.isInt(&m_b);
>      bool oc = c.isInt(&m_c);
>      return oa && ob && oc;
> }
>
> vs
>
> // "something else"
> bool S::parse(QString a, QString b, QString c)
> {
>      return a.isInt(&m_a) && b.isInt(&m_b) && c.isInt(&m_c);
> }

Those versions have the same “no strong-guarantee” problem.

> If so, the "Optional" variant doesn't look much simpler than any
> of the others, even less so when one doesn't accept the use of
> 'auto' there.

I never advocated that Optional makes for shorter code :) (yup, I know 
others have, but i disagree with that statement). However, i think 
Optional is a mean for *better* code. Your examples confort me in this 
thinking : with Optional, strong-guarantee is achieved within the same 
code amount. Without, it requires at least twice as more code than the 
unsafe version.

Regards,

Julien