fhem.pl: $hash->{CHANGED} init bug

[RichardCZ]

HoBo fliegt mir natürlich ständig um die Ohren (*), aber das ist gut so. 

So auch in addEvent

    push @{$hash->{CHANGED}}, $event;

wo ich beim Start eine Meldung "not an Array" bekam. Nach einigem hin und her sehe ich also, dass $hash->{CHANGED} bei mit {} eine hashref ist.
Glücklicherweise erinnerte ich mich an eine Änderung in readingsBeginUpdate, wo folgender sicher fehlerhafter Code steht:

https://svn.fhem.de/trac/browser/trunk/fhem/fhem.pl?rev=21548#L4621

ich dachte - weil ich ja alles weiß - ich ändere das zu

$hash->{CHANGED}            //= {};

Boom - und Hobo fliegt auf die Schnauze.

$hash->{CHANGED}            //= [];

ist natürlich richtig. Bei dem gegenwärtigen FHEM Code ist es vorher undef und nachher undef, aber der Code kommt damit zurecht.
Nuja. Funktioniert ja, braucht man in FHEM nix ändern, aber ich muss.


(Bei diesem Text wurden 3 bissige Bemerkungen ausgelassen - diese nicht eingeschlossen)

(*) dafür tut TTS

[rudolfkoenig]

Glücklicherweise erinnerte ich mich an eine Änderung in readingsBeginUpdate, wo folgender sicher fehlerhafter Code steht:

Welche Probleme kriege ich wegen diesen sicher fehlerhaften Code?

[herrmannj]

Rudi. welcher Aufwand entsteht das zu fixen? 

[CoolTux]

Ich bin alle $hash->{CHANGED} durchgegangen und überall wird davon ausgegangen das es sich um ein Array handelt. Daher denke ich das der erwähnte Code in der Tat fehlerhaft ist.
Sehe ich das richtig?

[RichardCZ]

Welche Probleme kriege ich wegen diesen sicher fehlerhaften Code?

So gesehen keine. Und so gesehen ist vielleicht der bessere Fix als mein //= [] tatsächlich die Zeile einfach zu entfernen.
Es ist eine NOP.

[rudolfkoenig]

Ich meine auch, dass [] "richtiger" ist, aber der aktuelle Code funktioniert scheinbar, und ich wuesste gerne warum, und welche Nebeneffekte es hat, wenn ich es aendere: diese Stelle betrifft sicher alle.

Und so gesehen ist vielleicht der bessere Fix als mein //= [] tatsächlich die Zeile einfach zu entfernen.

Ich glaube nicht:

Code Auswählen Erweitern

fhem> { push $defs{global}{CHANGED}, "on" }
Not an ARRAY reference at (eval 24) line 1.

Nachtrag: richtig geschrieben funktioniert es doch, man kann also vmtl. diese Zeile wirklich entfernen.

Code Auswählen Erweitern

fhem> { push @{$defs{global}{CHANGED}}, "on" }
1


[herrmannj]

Na changed ist doch immer ein Array? Also muss es ein Typo sein. Oder sehe ich das zu naiv

[CoolTux]

Na changed ist doch immer ein Array? Also muss es ein Typo sein. Oder sehe ich das zu naiv

So sehe ich das auch. Wieso es aktuell "funktioniert" ist hier die Frage. 

[rudolfkoenig]

CHANGED ist ein Array-Referenz, was man mit [] initialisiert, mit () initialisiert man Arrays.
Ein hash kann nur Referenzen enthalten, keine Arrays.

Meine aktuelle Theorie: () ist einfach nichts (ref sagt das auch so ), aber @{} legt auch bei nichts ein Array (samt Referenz) an, deswegen funktioniert push(@{defs{global}{CHANGED}, "on").
Es sei denn, ich uebersehe was, und dann haben wir morgen hier was zu tun.

[CoolTux]

Also in Verbindung mit readingsBeginUpdate folgt ja ein readingsBulkUpdate in der Funktion finde ich keinen Verweis auf $hash->{CHANGED} und in der dann folgenden readingsEndUpdate wird $hash->{CHANGED} einfach nur gelöscht.
Wenn jetzt nicht noch irgendwas dazwischen kommt kann ich für mich erklären wieso kein Problem auf taucht.

Ich habe aber sicherlich was übersehen zwischen den 3 Routinen. 

[RichardCZ]

Ich verstehe momentan ehrlich gesagt auch nicht warum der Code einem nicht ständig um die Ohren fliegt. Ohne Ironie - ich kapier's nicht.

3649       if($hash->{CHANGED}) {
3650         push @{$hash->{CHANGED}}, $newState;
3651       } else {
3652         $hash->{CHANGED}[0] = $newState;
3653       }
3654     } elsif(!defined($hash->{CHANGED})) {
3655       return "";
3656     }

if $hash->{CHANGED} - was soll das sein?

Das Ding könnte jetzt 1 sein, {blah => 2}, 'Ringelpietz' und der if ist zufrieden.
Die nachfolgende Dereferenzierung muss aber in dem Fall zwingend die Grätsche machen.

=> Ich finde, es sollte if (ref $hash->{CHANGED} eq 'ARRAY') lauten

$hash->{CHANGED}[0] = $newState; - auch das verstehe ich nicht

Also wenn jetzt $hash->{CHANGED} undef war ... mal testen ... hey wow - ich lerne auch noch was dazu:

$hash->{CHANGED}[0] =  $newState ;

macht dasselbe wie

$hash->{CHANGED} = [ $newState ];

vermutlich ist ersteres eine Art Do-What-I-Mean Perl Autovivification. Und das mit use strict und use warnings! Ich staune.
Aus der Wartbarkeitsperspektive geht das ja gar nicht, weil einen undef Wert mal eben als Listref zu dereferenzieren und diesem listref-element-0 was zuweisen. Da zuckt der Laie nur mit den Schultern und es staunt der Experte.

Also bitte wenn das jemand mal macht, die letztere Form. Und ich gehe jetzt ein paar akademische Diskussionen mit Kollegen führen.

[CoolTux]

=> Ich finde, es sollte if (ref $hash->{CHANGED} eq 'ARRAY') lauten

Da stimme ich Dir zu. Aber. Es funktioniert weil der Autor genau weiß was $hash->{CHANGED} wann ist und das es wenn es definiert ist immer eine Array Referenz ist. Wäre das Ergebnis unbekannt würde es hier sicherlich krachen.
Hatte ich schon ganz oft.

[rudolfkoenig]

Ich habe das () aus dem ersten Beitrag nach [] geaendert.
Ich will lieber die explizite Zuweisung haben.

[RichardCZ]

       if ($hash->{CHANGED}) { # It gets deleted sometimes (?)

Interessanter Kommentar in DoTrigger