Fixed interpret to C++ by uselessgoddess · Pull Request #68 · linksplatform/Disposables

uselessgoddess · 2021-02-14T14:08:47Z

Могут быть проблемы с подключением(include) из-за странной страсти автора к #pragma once и надеждой на то, что всё будет зависеть только от одного файла(куда собственно и будет includ'иться ).

…получилось сделать из-за довольно странной череды событий)

…получилось сделать из-за довольно странной череды событий) (См. TODO comment)

(стек на мьютексах? кому он нужен? это ведь детали реализации всё-таки) ((все странности помечены знаменитыми TODO comments))

konard

Let's start a review.

konard · 2021-02-20T15:48:50Z

cpp/Platform.Disposables/Platform.Disposables.h

-//#include <Platform.Hashing.h>

-#include <Platform.Delegates.h>
-#include <Platform.Exceptions.h>


These includes should be restored.

konard · 2021-02-20T15:50:17Z

cpp/Platform.Disposables/IDisposableExtensions.h

@@ -1,13 +1,20 @@
-namespace Platform::Disposables
+#ifndef DISPOSABLES_IDISPOSABLE_EXTENSIONS_H
+#define DISPOSABLES_IDISPOSABLE_EXTENSIONS_H


At the moment there is no need to use header guards in each class file. The template library is included in one piece as a cpp/Platform.Disposables/Platform.Disposables.h file.

konard · 2021-02-20T15:51:09Z

cpp/Platform.Disposables/EnsureExtensions.h

    class EnsureExtensions
    {
-        public: static void NotDisposed(Platform::Exceptions::ExtensionRoots::EnsureAlwaysExtensionRoot root, IDisposable &disposable, std::string objectName, std::string message)
+        public: static void NotDisposed(Platform::Exceptions::ExtensionRoots::EnsureAlwaysExtensionRoot root, IDisposable* disposable, std::string objectName, std::string message)


Why did you change the reference to pointer here?

I can fix this

I cannot fix

Why?

Because any class with undefined virtual methods(virtual ... foo (...) = 0;) can only be a pointer. IDisposable is exactly like this

I did an experiment. I found that it is possible to pass abstract class by reference.

It was based on an idea from Stack Overflow, which I found using С++ abstract class by reference search request in Google.

konard · 2021-02-20T15:54:45Z

cpp/Platform.Disposables/Disposable[T].h

+            // о нет так можно делать только в visual c++
+            // __super::Dispose(manual, wasDisposed)
+            // ----------------------------------
+            // base.Dispose(manual, wasDisposed);


You can try to use Disposable<>::Dispose(manual, wasDisposed); here.

konard · 2021-02-20T15:55:24Z

cpp/Platform.Disposables/Disposable[T].h

+
+namespace Platform::Disposables
 {
-    template <typename ...> class Disposable;


Why did you remove default template root?

konard · 2021-02-20T15:56:02Z

cpp/Platform.Disposables/Disposable[TPrimary, TAuxiliary].h

+
+namespace Platform::Disposables
 {
-    template <typename ...> class Disposable;


Why did you remove default template root?

konard · 2021-02-20T15:57:48Z

cpp/Platform.Disposables/DisposableBase.h

            return false;
        }

-        static DisposableBase() { std::atexit(OnProcessExit); }


Why did you remove static constructor? OnProcessExit method should be called once per all instances of DisposableBase.

I probably should have commented on that.
There are no static constructors in C++

But you can do it like this: https://stackoverflow.com/questions/1197106/static-constructors-in-c-i-need-to-initialize-private-static-objects

Yes you can add an inner class with StaticInitializer name. This class should contain only constructor. And it should be immediately embedded as a static field.

konard · 2021-02-20T15:58:31Z

cpp/Platform.Disposables/DisposableBase.h

-}
+}
+
+std::stack<std::weak_ptr<Platform::Disposables::DisposableBase>> Platform::Disposables::DisposableBase::_disposablesWeekReferencesStack = std::stack<std::weak_ptr<DisposableBase>>();


Does this line complied with no errors?

Yes, static fields in C++ leave much to be desired. This seems to be solved via 'Instance'.
But it works. As my favorite singer sings: "Wet flesh, good size"

We have a template class library, and we use C#-like style. So, please, move this initialization inside class definition scope.

In C++, you can't initialize any static fields inside a class. In an ideal implementation, this should generally be done in a file .cpp of the following format:

//in the CLASS.cpp file #include "CLASS.h" /*type*/ CLASS::static_field = ...;

It is possible if you use inline keyword, look at the example.

konard · 2021-02-20T15:59:41Z

cpp/Platform.Disposables/DisposableBase.h


-        ~DisposableBase() { Destruct(); }
+        //TODO: завязывайте с вызовом всякой виртуальщины в конструкторе/деструкторе
+        //~DisposableBase() { Destruct(); }


Why we cannot call virtual methods in destructor?

Because clang says "Inspection' Virtual call from constructor or destructor' "

In addition, you can look at these two codes:
https://rextester.com/PUOW20883 (С++)
https://rextester.com/TWL32777 (C#)

In principle, this problem can be easily solved, but I'm not sure that it will be in the style of C# Disposables
For example, something like this:

class DisposableWrapper { DisposableBase* disposableObject; public: DisposableWrapper(DisposableBase* disposableObject) : disposableObject(disposableObject) {} ~DisposableWrapper() { if(disposableObject != null) disposableObject->Dispose(...); else ..... delete disposableObject; } };

Ok, we can skip it at the moment, and force child implementations to call Destruct method in the destructor. Anyway I think we can still call the Destruct method in the destructor, but we also should do it in any implementation of DisposableBase. We also should make base destructor virtual to make child destructor able to run.

I'll try to figure it out

konard · 2021-03-01T10:38:55Z

cpp/Platform.Disposables/IDisposable.h

@@ -7,4 +13,6 @@

        virtual void Destruct() = 0;
    };


Abstract virtual destructor should be added here and in all other abstract classes (including System::IDisposable).

See also linksplatform/Interfaces#56

poul250 · 2021-03-03T11:02:39Z

cpp/Platform.Disposables/DisposableBase.h

-        protected: DisposableBase()
+        public: DisposableBase()
        {
+            std::shared_ptr<DisposableBase> this_ptr = std::shared_ptr<DisposableBase>(this);


This is a bad and not working solution. If we create an object of this class on the stack, then after exiting the constructor, its destructor will be called, and the program will crash due to std::terminate, because we will try to free memory on the stack (invalid operation). If We create an object in the form std::make_shared<T>(), then we will have two different counters for the number of pointers, which will lead to the fact that we will try to free memory in the heap twice (invalid operation).
I suggest doing the following:

Additionally inherit this class from std::enable_shared_from_this

class DisposableBase : public std::enable_shared_from_this<DisposableBase>, public IDisposable { /*...*/};

In constructor safely get weak_ptr

auto this_weak = weak_from_this();

poul250 · 2021-03-06T15:07:21Z

cpp/Platform.Disposables/Disposable.h

+            auto result = object->TryDispose();
            if (result)
            {
                object = 0;


What's the point of nulling a local variable? I'm sure the compiler will even cut out this piece of dead code.

In C#, it forces an object to be a candidate for garbage collection if no one else reference it. It also prevents the object from being used again (a safety from stupid mistakes). Translation is incorrect here, should be *object = nullptr and argument should be T** object

konard · 2021-03-07T18:07:30Z

At the moment it is decided that the task should be not completed.
#49

uselessgoddess added 3 commits February 7, 2021 19:01

(Для начала стоит конечно проверить работоспособность, что у меня не …

bfed46e

…получилось сделать из-за довольно странной череды событий)

(Для начала стоит конечно проверить работоспособность, что у меня не …

e5bc170

…получилось сделать из-за довольно странной череды событий) (См. TODO comment)

В принципе всё

ca2585f

(стек на мьютексах? кому он нужен? это ведь детали реализации всё-таки) ((все странности помечены знаменитыми TODO comments))

konard reviewed Feb 20, 2021

View reviewed changes

konard reviewed Mar 1, 2021

View reviewed changes

poul250 reviewed Mar 3, 2021

View reviewed changes

poul250 reviewed Mar 6, 2021

View reviewed changes

FreePhoenix888 changed the base branch from master to main March 27, 2022 07:14

FreePhoenix888 mentioned this pull request Mar 27, 2022

Change organization pull requests base branch from master to main linksplatform/Data.Doublets#340

Closed

Conversation

uselessgoddess commented Feb 14, 2021

Uh oh!

konard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konard Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konard Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konard Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poul250 Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konard Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konard commented Mar 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

konard Mar 1, 2021 •

edited

Loading

konard Feb 28, 2021 •

edited

Loading

konard Feb 28, 2021 •

edited

Loading

poul250 Mar 3, 2021 •

edited

Loading

konard Mar 6, 2021 •

edited

Loading