Guida BugFix Funzione RemoveAttributeType

Stato
Discussione chiusa ad ulteriori risposte.

luzzi

Utente Jade
30 Ottobre 2008
1,957
96
961
855
Che conseguenze può avere questo bug?
Questa funzione può essere dei seguenti problemi:
- Cercando di eliminare su un item un determinato bonus, questo bonus seppur presente non verrà eliminato.
Esempio:
C++:
pItem->RemoveAttributeType(APPLY_SKILL_DAMAGE_BONUS);
-In condizioni particolari questa funzione può portare al crash del core o dell'intera macchina.
Esempio (Supponiamo che esista un item che come secondo bonus abbia HP):
Codice:
pItem->RemoveAttributeType(APPLY_MAX_HP);

Quali rev sono affette da questo bug?Come faccio a capire se ho questo bug?

Attualmente tutte le revisioni sia pubbliche che private sono affette da questo bug.
Puoi verificare di essere affetto da questo bug se la tua funzione che si trova in item_attribute.cpp
è così scritta:
C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeType(index);
}


Il Fix

Il fix è estremamente semplice:

C++:
//Funzione Buggata
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeType(index);
}

//Funzione Fixata
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeAt(index);
}
 
Ultima modifica:
Che conseguenze può avere questo bug?
Questa funzione può essere dei seguenti problemi:
- Cercando di eliminare su un item un determinato bonus, questo bonus seppur presente non verrà eliminato.
Esempio:
C++:
pItem->RemoveAttributeType(APPLY_SKILL_DAMAGE_BONUS);
-In condizioni particolari questa funzione può portare al crash del core o dell'intera macchina.
Esempio (Supponiamo che esista un item che come secondo bonus abbia HP):
Codice:
pItem->RemoveAttributeType(APPLY_MAX_HP);

Quali rev sono affette da questo bug?Come faccio a capire se ho questo bug?

Attualmente tutte le revisioni sia pubbliche che private sono affette da questo bug.
Puoi verificare di essere affetto da questo bug se la tua funzione che si trova in item_attribute.cpp
è così scritta:
C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeType(index);
}


Il Fix

Il fix è estremamente semplice:

C++:
//Funzione Buggata
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeType(index);
}

//Funzione Fixata
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    return index != -1 && RemoveAttributeAt(index);
}

Mi permetterei , con tutto il rispetto dovuto , di consigliare una simile ma diversa soluzione , derivata dalla tua :

C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
  
    if(index == -1)
        return false;
    else
        RemoveAttributeAt(index);
  
    return index != -1 && RemoveAttributeType(bType);
}


Questo perchè credo che RemoveAttributeType serva a togliere tutti gli attribute di un determinato type (ecco perchè la recursività) , nonostante ovviamente al momento non sia possibile avere due volte lo stesso tipo di bonus su uno stesso item.
In realtà è più per una questione di completezza , non è un cambiamento necessario.
Ovviamente complimenti a @luzzi per aver scovato il bug , era ben nascosto ma pericolosissimo visto il loop che genera..
Tanti saluti :)
 
Mi permetterei , con tutto il rispetto dovuto , di consigliare una simile ma diversa soluzione , derivata dalla tua :

C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
 
    if(index == -1)
        return false;
    else
        RemoveAttributeAt(index);
 
    return index != -1 && RemoveAttributeType(bType);
}


Questo perchè credo che RemoveAttributeType serva a togliere tutti gli attribute di un determinato type (ecco perchè la recursività) , nonostante ovviamente al momento non sia possibile avere due volte lo stesso tipo di bonus su uno stesso item.
In realtà è più per una questione di completezza , non è un cambiamento necessario.
Ovviamente complimenti a @luzzi per aver scovato il bug , era ben nascosto ma pericolosissimo visto il loop che genera..
Tanti saluti :)



Caro @Ikarus_ come tu stesso hai notato non è possibile aggiungere più di un bonus dello stesso tipo. Motivo per cui quella funzione non e' stata creata per rimuove più di un attribute.

Detto questo

C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
 
    if(index == -1)
        return false;
    else // else condition non e' necessaria qui. e' implicita. Oltretutto if ed else non modificano il comportamento di quello che c'è dopo.
        RemoveAttributeAt(index);
 
    return index != -1 && RemoveAttributeType(bType); // index != -1 non ha senso qui è always true.
}

Seppur sconsigliata
C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
 
    if(index == -1)
        return false;

    RemoveAttributeAt(index);
    return RemoveAttributeType(bType);
}


Concludo ricordandoti che

C++:
bool CItem::RemoveAttributeType(BYTE bType)

E' una funzione boleana e bisogna dare importanza al valore che ritorna cosa che non viene fatta nel codice da te propoposto.

Giusto un esempio

C++:
if(item->RemoveAttributeType(APPLY_NORMAL_HIT_DAMAGE_BONUS))
   sys_log(0, "E' stato rimosso il bonus");

La tua funzione non permetterà mai la distinzione sulla rimozione del bonus perchè ritorna sempre false.
 
Caro @Ikarus_ come tu stesso hai notato non è possibile aggiungere più di un bonus dello stesso tipo. Motivo per cui quella funzione non e' stata creata per rimuove più di un attribute.

Detto questo

C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
 
    if(index == -1)
        return false;
    else // else condition non e' necessaria qui. e' implicita. Oltretutto if ed else non modificano il comportamento di quello che c'è dopo.
        RemoveAttributeAt(index);
 
    return index != -1 && RemoveAttributeType(bType); // index != -1 non ha senso qui è always true.
}

Seppur sconsigliata
C++:
bool CItem::RemoveAttributeType(BYTE bType)
{
    int index = FindAttribute(bType);
    //return index != -1 && RemoveAttributeType(index); //@fix accidental recursive methods
 
    if(index == -1)
        return false;

    RemoveAttributeAt(index);
    return RemoveAttributeType(bType);
}


Concludo ricordandoti che

C++:
bool CItem::RemoveAttributeType(BYTE bType)

E' una funzione boleana e bisogna dare importanza al valore che ritorna cosa che non viene fatta nel codice da te propoposto.

Giusto un esempio

C++:
if(item->RemoveAttributeType(APPLY_NORMAL_HIT_DAMAGE_BONUS))
   sys_log(0, "E' stato rimosso il bonus");

La tua funzione non permetterà mai la distinzione sulla rimozione del bonus perchè ritorna sempre false.
Bhe che dire , hai puntualizzato punti importanti, quindi basta richiamare recursivamente fuori dal return , ritornando true subito dopo e anche il ritorno torna ad essere sensato nonostante la recursivitá..
(L'orario del mio primo messaggio penso parli chiaro ahah)

Volevo chiederti:

Nella tua funzione ritorni

index != -1 && RemoveAttributeAt(index)

So che il cpp ha una proprietà grazie alla quale non verifica le ipotesi non necessarie per cui nel caso index sia proprio -1 non dovrebbe verificare RemoveAttributeAt in quanto si conosce già il risultato dell'espressione che è false...

So che questa ottima proprietà funziona per gli if, ora la mia domanda è , funziona anche nel caso di un return?

Perdonami se la domanda ti pare scontata .. Sono un autodidatta in programmazione e tante volte delle cose semplici mi sfuggono..

Inviato dal mio LG-H870 utilizzando Tapatalk
 
Bhe che dire , hai puntualizzato punti importanti, quindi basta richiamare recursivamente fuori dal return , ritornando true subito dopo e anche il ritorno torna ad essere sensato nonostante la recursivitá..
(L'orario del mio primo messaggio penso parli chiaro ahah)

Volevo chiederti:

Nella tua funzione ritorni

index != -1 && RemoveAttributeAt(index)

So che il cpp ha una proprietà grazie alla quale non verifica le ipotesi non necessarie per cui nel caso index sia proprio -1 non dovrebbe verificare RemoveAttributeAt in quanto si conosce già il risultato dell'espressione che è false...

So che questa ottima proprietà funziona per gli if, ora la mia domanda è , funziona anche nel caso di un return?

Perdonami se la domanda ti pare scontata .. Sono un autodidatta in programmazione e tante volte delle cose semplici mi sfuggono..

Inviato dal mio LG-H870 utilizzando Tapatalk

In realtà ciò che basta non è mai la soluzione migliore. Ti invito a leggere "the clean cloder".

Detto questo non è una propietà di cpp o almeno non solo.
Quando si calcola un valore boleano tramite operatori logici viene applicato il seguente ragiornamento

&&
Se la prima è vera valuta la seconda altrimenti sicuramente è falsa.

||

Se la prima è falsa allora valuta la seconda altrimenti è sicuramente vera.
 
Stato
Discussione chiusa ad ulteriori risposte.