Eigener kopier- und auseinandernehmbarer Composite-Tree - Review?



  • naja, nen value hier und ein static_assert weniger und so, aus der Hüfte geschossen.



  • Ich finde std::unique_ptr<..> im Interface gut, weil es eine fehlerhafte Benutzung verhindert (oder mindestens erheblich erschwert) und weil es zeigt, was die Absicht ist (z.B. Besitz bei Add(..) übergeben/übernehmen).

    Mit std::make_unique<..>(..) aus C++14 oder einem selbstgemachten make_unique<..>(..) sieht das Ganze dann sehr ordentlich aus. Der Vorschlag von decimad finde ich auch in Ordnung.

    Edit (1)

    Eisflamme schrieb:

    Das bläht den Code in meinen Augen ziemlich auf, findest du das ggü. der leichten Intuition, wie die Speicherverwaltung ist, tragbar?

    Ich sehe das genau umgekehrt: Der Code wird minimal, falls überhaupt aufgebläht und die Verwendung der Interfaces wird dadurch klarer. Die Absicht wird gezeigt.

    Eisflamme schrieb:

    Und: Wenn ich den Pointer durch move erstmal "Add"e, kann ich später nicht mehr denselben Zeiger nutzen, um dort Kinder einzufügen.

    Das mache ich jeweils so, dass ich eine Referenz auf das eingefügte Element zurückgebe - so kannst du die Einfüge-Operationen sogar verknüpfen.

    Edit (2)
    BTW: Mit der unique_ptr<..>-Lösung, brauchst du auch keine Kommentare mehr, die Erklären, wie der Besitz geregelt ist.



  • Hi,

    okay, überzeugt. 🙂 Und ::type muss noch ans Ende. Also die neue Version:

    http://ideone.com/o3b8T0

    In Visual Studio 2013 kompiliert das. Aber das enable_if bei Add substituted nicht bei Ideone, als könnte er TreeType nicht zu TreeType konvertieren, was natürlich sinnlos ist. Wenn ich die enable_if-Zeile bei Add auskommentiere, funktioniert es wie erwartet. Jemand eine Idee? 😞



  • Ich hab kurz überlegt, ob er beim SFINAE versucht TreeNode (Klasse) mit TreeNode (forw. declaration) zu vergleichen, aber innerhalb der Klasse sollte er ja wohl TreeNode kennen, also kann es das nicht sein...



  • Es funktioniert zumindest mit std::is_base_of. Ich glaube, std::is_convertible erlaubt diese Konvertierungen nicht, weil für was es gedacht ist, slicing verhindern möchte.
    Da GetRootNode aber eine Referenz auf den Knoten zurückliefert, schmiert Dein Programm ab, wenn aus dem Baum herausgemoved wurde... Zumindest verwendet Dein Testcode ihn dann munter weiter.



  • Ich verstehe den Zusammenhang zwischen enable_if und der RootNode-Referenz gerade nicht.

    Herausmoven aus dem Baum ist natürlich so ne Sache, dann ist der alte Baum invalid. Man könnte den alten Baum auch wiederbeleben, indem man den rootNode neu erzeugt.

    Was du damit meinst, dass TreeNode munter weiterbenutzt wird, verstehe ich nicht. Nach Move hat sich TreeNode doch nicht verändert, der lebt doch im unique_ptr. Könntest du vielleicht auf die kritische(n) Codezeile(n) verweisen?


  • Mod

    Eisflamme schrieb:

    In Visual Studio 2013 kompiliert das. Aber das enable_if bei Add substituted nicht bei Ideone, als könnte er TreeType nicht zu TreeType konvertieren, was natürlich sinnlos ist. Wenn ich die enable_if-Zeile bei Add auskommentiere, funktioniert es wie erwartet. Jemand eine Idee? 😞

    Der Kopierkonstruktor ist als protected deklariert. Und

    §14.3/3 schrieb:

    For a template-argument that is a class type […], the template definition has no special access rights to the members of the template-argument.

    Wenn ich e.g. den Kopierkonstruktor als public deklariere, bekomme ich außerdem einen Segfault.

    PS: Code bitte stets via Coliru posten.


  • Mod

    decimad schrieb:

    std::is_convertible erlaubt diese Konvertierungen nicht, weil für was es gedacht ist, slicing verhindern möchte.

    😕 is_convertible wird intern mit SFINAE realisiert. Es wird einfach versucht eine copy-initialization durchzuführen.

    libstdc++' <pre><code><type_traits></code></pre>, Zeile 1463 schrieb:

    template<typename _From, typename _To,
               bool = __or_<is_void<_From>, is_function<_To>,
                            is_array<_To>>::value>
        struct __is_convertible_helper
        { typedef typename is_void<_To>::type type; };
    
      template<typename _From, typename _To>
        class __is_convertible_helper<_From, _To, false>
        {
           template<typename _To1>
    	static void __test_aux(_To1);
    
          template<typename _From1, typename _To1,
    	       typename = decltype(__test_aux<_To1>(std::declval<_From1>()))>
    	static true_type
    	__test(int);
    
          template<typename, typename>
    	static false_type
    	__test(...);
    
        public:
          typedef decltype(__test<_From, _To>(0)) type;
        };
    

    Die copy-initialization wird mittels Initialisierung eines Funktionsparameters "simuliert", wobei die Funktion __test_aux<_To1> ist. Also die intuitive Implementierung. Und natürlich schlägt die Deduzierung in Zeile 14 fehl, da kein Move-Konstruktor existiert
    ( std::declval<_From1>() ist ein x- bzw. rvalue) und der Kopierkonstruktor, wie bereits erwähnt, nur protected .

    is_base_of wird i.d.R. durch ein Intrinsic realisiert, es geht aber natürlich auch anders, und zwar indem die Ambiguität einer Konvertierung ausgenutzt wird - access != visibility. Wie auch immer, is_base_of und is_convertible sind zwei völlig verschiedene Dinge.



  • Coliru? Nie gehört, probiere ich nächstes mal gerne aus. 🙂

    Also sollte is_base_of das Problem lösen?

    Der Segfault kommt daher, dass bei:

    Tree a;
    Tree b(std::move(a));
    

    a ungültig wird.

    Was ist denn hier das erwartete/präferierte Verhalten? Es ist ja in Ordnung zu sagen, der gemovete ist ungültig. Man könnte aber auch wieder als Root-Node einen TreeNode neu einfügen, dann ist der Baum gültig aber leer.



  • Ich hab' mir zu dem Zeitpunkt nicht den Quelltext von is_convertible angeschaut, was in ideone wohl auch nur schwerlich geht. In einer der online c++ referenzen steht aber, dass es wohl bei Rückgabewerten per Wert Anwendung finden soll. Dort würde ich erwarten, dass nicht einfach weggesliced wird. Daher stammte meine _Vermutung_ wie is_convertible ausgelegt ist, den einschränkenden Teil hast Du nun aber einfach aus dem Quote herausgenommen.

    Normalerweise erwartet man, dass das ausgemovete Objekt in einem leeren aber gültigen Zustand verbleibt, also dass Zerstörung usw. alles noch definiert ist. Und nicht mehr! Das soll ja zumeist für Parameterverkettungen usw. verwendet werden und ist dann hinterher nur noch als leere Hülle betrachtet.


Anmelden zum Antworten