Dalija Prasnikar

1 minute ago, Attila Kovacs said:

Actually I only handle exceptions where I have a plan B, otherwise I could only translate the messages to other languages.

What do you mean by plan B?

21 minutes ago, Lars Fosdal said:

So, basically

try
  var MyThing := TThing.Create;
  try
    MyThing.DoIt;
  finally
    MyThing.Free;
  end;
except
  // deal with it
end;

// instead of

var MyThing := TThing.Create;
try
  try
    MyThing.DoIt;
  except
    // deal with it
  end;
finally
  MyThing.Free;
end;

I prefer the latter since I can handle the problem in the context of which it occurs.

IMO, exceptions that you let "bubble up the stack" become increasingly difficult (Edit: and expensive) to manage, since you are less and less aware of the exception context, the further out you get.

 

If you want to handle the exception on site, then the second option is the wrong way to do it for several reasons. 

Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so. If you are fine to bubble up that exception further on and if except part (handling the exception) cannot raise exceptions, you don't need try..finally at all.

1 hour ago, Rollo62 said:

Right, but why does it feel so bad

You tell me. It doesn't feel bad to me at all 

I think you are overcomplicating. 

If you have class that requires some setup (that will commonly not be changed once object is created) to function properly then passing those parameters through constructor and raising exception in constructor is the best and simplest thing to do.

Use the approach that requires the least amount of code for safely using such class and where you cannot easily forget to do something vital. Usually that will be the best code.

4 minutes ago, Lars Fosdal said:

Also, if you accept that exceptions may occur in the constructor - doesn't that mean that this model is not generally applicable anymore, and you have to wrap that in an exception handler as well?

var MyThing := TThing.Create;
try
  MyThing.DoIt;
finally
  MyThing.Free;
end;

 

Actual purpose of try...finally in above code is to ensure cleanup of the MyThing instance if MyThing.DoIt raises exception. If constructor raises the exception there will be automatic cleanup and MyThing.Free will never be, nor it should be called.

2 minutes ago, Lars Fosdal said:

Which means that you must add exception handling in all levels of the destructor chain?

destructor TMyThing.Destroy;
begin
  try
    inherited;
  finally
    try 
      // destroy my things
    except
      // this was unfortunate
    end
  end;
end;

 

No, you don't.

If the instance is never constructed it will be nil in the destructor and you can just call Free on such instance. You only need to consider that any instance in the destructor can be nil, so if you are calling any other methods you need to check for nil before calling those in destructor.

11 minutes ago, Attila Kovacs said:

If those methods are time critical you can do a lazy initialization or validate outside or even avoid OOP or whatever, but I would not validate in the constructor if I don't have to.

Nobody said that you must validate in the constructor. It all depends whether this is critical or not and at which point.

There are no wrong or right answers here, it all depends how class functionality and how it is used. Each approach has downsides and upsides.

3 hours ago, Rollo62 said:

But are all kind of field types secured, like Pointers, [ Weak ] field, and whatsnot, ... ?

Yes. All fields are zero initialized.

3 hours ago, Rollo62 said:

My habit shall prevent any unexpected glitch, but is of course double-safety in most cases.

(Is this an AntiPattern maybe ? )

There is no double safety if something cannot ever happen. If you have extra initialization code then such unnecessary code can obscure other important code. Keep it simple. If you are not sure what is initialized and when, it is better to check and learn than to use code just in case.

3 hours ago, Rollo62 said:

I also used the "flat" try - finally here and there, but this always left a bad taste to me.

What happend if I use this for more than two variables, which often may happen in Bitmap related stuff ?

var
  LA, LB, LC: TRaiseOnMinus;
begin
  LC := nil;
  LB := nil;
  LA := nil;
  try
    LA := TRaiseOnMinus.Create( -2 );  		
    LB := TRaiseOnMinus.Create( -3 );                                                   
    LC := TRaiseOnMinus.Create( -3 );                                                   
    //some code
  finally
    LC.Free;
    LB.Free;
    LA.Free; 					
  end;
end;

Wouldn't it be OK as well, when I use them "flat", but take special care on managing them in the right order ( ABC - CBA ), like above ?

It is fine to construct as many instances as you need and protect them with single try...finally. You don't need to pay attention to the order of destruction, unless there is some special reason where one instance still holds reference to other one you are destroying sooner, where you might have problem with dangling pointers. 

3 hours ago, Rollo62 said:

Why should I use one Create outside the try scope ?

There is no functional difference, only one nil assignment more in your case - if you feel like you can more easily follow the code if multiple instances are all initialized before try..finally, that is fine.

Remember, try...finally is used just for cleanup in case of exception, not for handling the exception. If exception is raised inside try..finally it will be propagated to the next exception handler block - try...except just the same it would if it is raised outside try..finally.

Again, if there is exception raised in constructor, assignment to the variable will never happen. So if LA is nil and the you call LA := TRaiseOnMinus.Create(-2) in finally block LA will still be nil. Automatic cleanup for that unassigned object instance is happening in hidden code inserted by compiler. If the instance is successfully created, then automatic cleanup will not happen.

3 hours ago, Rollo62 said:

So I would like to find the best answer or rule-of-thumb for this case, could be like this:

- never raise Exception on purpose, it's evil

- never raise Exception on purpose, it's an antipattern ( because ... see GOF page xxx )

- you can raise Exceptions safely, if you do prepare this and that

- if you always do it like this, you guaranteed to never ever run into any issues.

Raising exceptions in constructor is not evil nor bad practice. It is perfectly fine thing to do.

Whether you will raise exception in constructor or not, is matter of each particular class functionality. If you pass invalid parameter to a constructor it makes sense that such constructor raises exception, because otherwise your object is not properly functional instance, If you don't raise exception then, you would have to sprinkle your code all over inside that class and where you use it with checks for that particular invalid condition. When you have some condition that causes failure failing soon is always better than failing later on. If you have some reasons not to raise exception or there is nothing special that would require raising exception that is also fine. Constructing objects can always raise OutOfMemory so you always need to be prepared to do the cleanup anyway.

Take for instance TFileStream - if constructor fails to open or create file it will raise an exception. And you can handle that failure at that point. If you don't have to handle it at that point, that means you have opened file too soon. 

Some bullet points:

• if Assigned(Obj) then Obj.Free is redundant, you can just call Obj.Free - Free is static and can be called on nil instance and also Free has nil check within before calling destructor
• constructors are allowed to raise exceptions
• destructors must never raise exceptions (without handling them within with try...except)
• destructors can be called on half constructed instance (if the constructor fails, destructor chain will be automatically called) and must be able to handle such scenarios without causing trouble - raising exceptions
• when constructor fails assignment to variable will never happen
• local object variables are never automatically initialized and they can contain random garbage
• object instance fields are always zero initialized when constructor chain is being called

Because of the above correct construction pattern is

var
  LA: TRaiseOnMinus;
begin
  LA := TRaiseOnMinus.Create( -2 );  		
  try
    //some code
  finally
    LA.Free; 					
  end;
end;

With multiple constructors there are multiple options, but I prefer using only single try...finally because destructors must not raise exceptions, single try finally is faster and cleaner

var
  LA, LB: TRaiseOnMinus;
begin
  LB := nil;
  LA := TRaiseOnMinus.Create( -2 );  		
  try
    LB := TRaiseOnMinus.Create( -3 );                                                   
    //some code
  finally
    LB.Free;
    LA.Free; 					
  end;
end;

Correct constructor and destructor for your class would be:

constructor TRaiseOnMinus.Create( AParam : Integer );
begin
    inherited;
    if AParam < 0 then
        raise Exception.Create( 'Bum' );                 
    FMyList := TStringList.Create;                 
end;

destructor TRaiseOnMinus.Destroy;
begin
  FMyList.Free;
  inherited;
end;

Only if you need to do some additional work with FMyList in destructor, then you must check it for nil before using it. In such cases you can find call to Free within the same check, but not because it wouldn't work outside.

destructor TRaiseOnMinus.Destroy;
begin
    if Assigned(FMyList) then
      begin
        FMyList.DoSomethingImportant;
        FMyList.Free;
      end;
    inherited;
end;

28 minutes ago, dummzeuch said:

Isn't there a configuration option, how many recent files the IDE remembers and shows in this menu? Having to remove them manually can't really be the fix.

Yes, there is. I forgot about that.

I have a need for large number of projects and files and when I get into a trouble with the menu, it is easier for me to remove specific files than to have smaller number of files remembered.

43 minutes ago, alogrep said:

Hello.

Does anybody know how to fix this? 

Suddenly when I do File -> Open recent the listbox appears off the top ot the screen ad I can only see a few recent files (none of the Projects). 

This issue seems to happen when you have too many files in the recent menu. Open properties - most bottom option in the menu and delete few files (how many depends on the size of your screen) from the recent list. That should fix it. If it is not fixed, you will need to remove more files.

Reported as https://quality.embarcadero.com/browse/RSP-36702

22 minutes ago, Fr0sT.Brutal said:

Yep you're right. However, there's too much of JS-ism.

Instead of anonymous methods, you can also use plain TNotifyEvent and pass list item as Sender. 

Also if you don't like the code in completion handler because of TThread.Queue, this call can be moved to UpdateItem. There are many ways to write this functionality, this is just one. 

14 minutes ago, Fr0sT.Brutal said:

I'd additionally untie processing class from UI listview item completely. Processor just doesn't need to know about UI (unless it deals with it directly, like bg rendering or similar - but here it's not the case I suppose).

But it is completely decoupled in that code. The only connection is in attached completion handler that can hold any kind of code and is not part of processing class.

37 minutes ago, David Schwartz said:

But if class instances are attached to the .Data properties of the ListItems, then they'd need to know which LI they're attached to in order to update them, right?

 

I'm thinking something like this to kick them all off:

 

  for var li in lview1.Items do
  begin
  
    TMyClass( li.Data ).Execute( li ); 
    
  end;

Yes, you can use that approach, but it is not flexible because your processing function depends on the specific UI type.

37 minutes ago, David Schwartz said:

Would you create the task  inside of the .Execute method?

 

Or around that line? (suggesting that the 'li' parameter would probably not need to be injected)

 

Would it make much of a difference? 

 

I wouldn't create task inside Execute method because processing functionality itself should be implemented as clean synchronous code - that gives you more flexibility to combine its functionality as needed.

I would use something like:

  TMyClass = class
  public
    procedure Execute(OnCompleted: TProc);
  end;


procedure TMyClass.Execute(OnCompleted: TProc);
begin
  // process data
  //...
  if Assigned(OnCompleted) then
    OnCompleted;
end;

procedure TMyForm.ProcessItem(li: TListViewItem);
begin
  TTask.Run(
    procedure
    begin
      TMyClass(li.Data).Execute(
        procedure
        begin
          TThread.Queue(nil,
          procedure
          begin
            UpdateItem(li);
          end);
        end);
    end);
end;

procedure TMyForm.UpdateItem(li: TListViewItem);
begin
  li. ...
end;

procedure TMyForm.ProcessView;
begin
  for var li in lview1.Items do
  begin
    ProcessItem(li);
  end;
end;

4 minutes ago, David Schwartz said:

If this were inside of a class, I don't think the logic for updating the ListView belongs there.

 

Which means that the OnTerminated proc there needs to trigger a common event handler, passing in an ID for which line / ListItem that thread is processing and perhaps a status indicator. Which is pretty much the same as using the queue you suggested.

Or ... this whole bit of code is in a form method that's called and passed each ListItem where the .Data property has an object to be processed, and it has a method in it that the Async method runs as a task. Then the OnTerminated part updates the ListView.

 

Which approach would you go with?

You are right that the processing logic and updating UI don't belong together. 

When I have processing functionality as part of the class (and I usually do) I also declare completion handlers in that class. And then you can easily attach any kind of additional logic when processing is finished - including updating UI. The most important part is that all individual pieces of code can be easily unit tested (except for UI) and that there is minimal gluing code where you can make mistakes when you put all of the code together.

37 minutes ago, David Schwartz said:

When you do most DB operations, there's an OnxxxCompleted event that's called.

 

With tasks, there's ... nothing. They just stop.

That is true, there is no OnCompleted event

37 minutes ago, David Schwartz said:

When each task finishes, I want to update the line in the ListView that says it's Finished and show the Elapsed Time. 

You can do that inside the task code. You can either send message with some messaging system or call appropriate code directly (make sure it is synchronized with the main thread for UI interaction). To free thread for further processing it is better to use TTask.Queue instead of TThread.Synchronize, but if you need to perform some cleanup, Synchronize will do as long as it is not too slow. Something like:

TTask.Run(
  procedure
  begin
    // your background work
    ...
    // task is completed
    TThread.Queue(nil,
      procedure
      begin
        // update the UI
      end);
  end);

Since Delphi 10.4 memory management across all platforms has been unified. That means Free and FreeAndNil behave on android just like then always behaved on Windows.

There is still DisposeOf, but it just calls Free now. You can change those calls, but you don't have to. The only place where you can have leaks on Android due to this change is if you have been relying on ARC to release your objects and you were just assigning nil to them or you were just letting them go out of scope.

1 hour ago, Keesver said:

Can you explain why supports/queryinterface can't be used?

Because of this https://stackoverflow.com/a/42305385/4267244

But, once you have determined your weak reference is valid, you can take strong reference out of it. Once you have strong reference you can do what you need with that reference.

procedure TObjectWithTest.QueueCallSafe([weak]AInterface: ITest);
begin
  TThread.ForceQueue(nil, 
    procedure 
    var
      Strong: ITest;
    begin
      if AInterface = nil then
        ShowMessage('nil') 
      else
        begin
          Strong := AInterface;
          ShowMessage('not nil');
        end;
  end);
end;

Again, this only works if there are no background threads involved.

30 minutes ago, Keesver said:

We are looking at safely handling queued calls. The problem is that under some circumstances the object used inside the queued method is freed before the method gets executed. This raises an exception. Can we use this construct to fix it:

Depends what you mean by fixing it?

If you are fine that under some circumstances you will get nil inside QueueCallSafe and you check for such situation then you have fixed it. 

Be aware that this code will only work if object is released in the context of main thread - Obj.Free line. If you have that kind of code in some background thread then code in QueueCallSafe will not be safe.

Just now, JkEnigma said:

Hi all,

Currently, I am using Free Pascal but I realized that Delphi has several possibilities that would come in handy for me. I would like to give the Community Edition a try. However, I still remember using the free version of Borland Delphi which was discontinued after Delphi moved to CodeGear. That is a long time ago and Embarcadero is another company, but still...

Does anybody have information about the long term availability of the Community Edition? 

There is no guarantee it will be available in the future. We can only hope it will be

7 minutes ago, Anders Melander said:

Ah, yes. I missed that. Easily solved though (I think):

Yes.... forget the fake interface approach... brain fart...

Possible solution for interface problem would be using fake interface instance as magic value as reference counting on that would not cause any problems (Note: I haven't tested such solution). 

function NopAddRef(Inst: Pointer): Integer; stdcall;
begin
  Result := -1;
end;

function NopRelease(Inst: Pointer): Integer; stdcall;
begin
  Result := -1;
end;

function NopQueryInterface(Inst: Pointer; const IID: TGUID; out Obj): HResult; stdcall;
begin
  Result := E_NOINTERFACE;
end;

const
  FakeInterfaceVTable: array [0 .. 2] of Pointer = (@NopQueryInterface, @NopAddRef, @NopRelease);
  FakeInterfaceInstance: Pointer = @FakeInterfaceVTable;

At this point, solution with lock seems like simpler approach if it makes sense to avoid unnecessary instance construction. On the other hand, if you are to write some universal lazy initialized container that will work on different types, then this solution even though more complicated would pay off.

8 minutes ago, Anders Melander said:

Yes, I originally had a test for nil before the CompareExchange but removed it "for clarity".

 

Why do you say it doesn't work with interfaces? After all an interface reference is just a pointer and the object construction and assignment should take care of the initial reference count.

var
  FSingleton: ISingleton = nil;

function Singleton: ISingleton;
const
  MagicValue = pointer(1);
begin
  if (FSingleton = nil) then // Hapy now? :-)
    if (TInterlocked.CompareExchange(pointer(FSingleton), MagicValue, nil) = nil) then
      FSingleton := TSingleton.Create;

  // Wait for value to become valid
  while (pointer(FSingleton) = MagicValuel) do
    YieldProcessor;

  Result := FSingleton;
end;

What am I missing?

It will crash at FSingleton := TSingleton.Create during assignment.

At that point FSingleton value is pointer(1). FSingleton is interface, and assigning new value will try to call _Release on any non-nil old value to maintain appropriate reference count and that old value is invalid address.

1 hour ago, Anders Melander said:

Okay. Here goes:

You can but don't need to do bit fiddling. We just need to store 3 different states in the pointer;

• Unassigned
• Assigned
• Locked - being assigned

It's easy to identify the first two states; If the pointer value is nil then the pointer is unassigned, if it's non-nil then it's assigned. But what about the locked state? Well, for a pointer to an object we can take advantage of the fact that memory allocations are aligned so the lower n bits will always be zero and can be (mis)used to store a lock flag. However since it's a pointer we can just as well just use a pointer value that are known never to be returned by the memory manager. For example ordinal 1.

var
  FSingleton: TSingleton = nil;

function Singleton: pointer;
const
  MagicValue = pointer(1);
begin
  if (TInterlocked.CompareExchange(FSingleton, MagicValue, nil) = nil) then
    FSingleton := TSingleton.Create;

  // Wait for value to become valid
  while (FSingleton = MagicValuel) do
    YieldProcessor;

  Result := FSingleton;
end;

That is better solution (it would require additional if to prevent calling CompareExchange for every access, similar to original example (full code)) , but it does not work with interfaces.

1 hour ago, Anders Melander said:

Now you're mixing two different arguments. I'm not opposing full pointer exchange. I'm opposing the optimistic object instantiation (which presumably will cause a lock in the memory manager but regardless will be costlier than doing an explicit lock).

Your example used additional lock, so it wasn't really clear what you are objecting to.

Also, with lazy initialization code path where possible unnecessary object construction happens is extremely rare situation and even if it does happen lock in memory manager not something that would bother me. Because it happens rarely, even cost of explicit lock is negligible so I never used that "potential" performance issue as something worth measuring and proving which solution is faster. 

The only considerable argument is how costly is the construction of the object that might get discarded, but using lock in memory manager is a poor way to prove that point.

17 hours ago, MarkShark said:

Also just as a note I see that Emba switched to using AtomicCmpExchange instead of InterlockedCompareExchangePointer in Delphi 11 (or perhaps earlier.)  Kind of interesting.

InterlockedCompareExchangePointer is part of Windows API. AtomicCmpExchange is added to provide cross-platform support. 

There is also TInterlocked class in System.SyncObjs that implements various static functions for performing interlocked operations, including CompareExchange that basically calls AtomicCmpExchange, but it is more convenient to use for some types.

17 hours ago, Anders Melander said:

It doesn't need to be, FTOH you can use the low bits of the pointer as a spin-lock flag.

And how exactly would you do that in this scenario?

Not to mention that you are opposing simple full pointer exchange because it is too "clever", and your solution would be bit fiddling.

17 hours ago, Anders Melander said:

That's rather condescending.  It's not that I'm not comfortable with it. It's just a solution I personally wouldn't use.

Comfortable, prefer, personally use... those are all the same reasons, just different words. You are reading into words way too much. You don't want to use something because you have subjective reasons. I have no problem with that and I am doing the same in many places. 

The only reason why am I here objecting, is you saying this is not good pattern and this is not true and it sends wrong message to others that are not familiar with this kind of code. By you saying this is bad code, someone else might avoid such coding pattern because they will wrongly think it is a bad one. 

The only place where I wouldn't use this pattern is in places where constructed object is extremely heavy and where constructing object that would possibly be immediately discarded would consume plenty of resources or would take a lot of time.

17 hours ago, Anders Melander said:

I would say that what constitutes "good" is subjective and not defined by "because it is".

I already said why it is good, but I will repeat it.

It is lock-free, so the whole operation can be done with single reference (I am not talking about local variables, but broader scope where final reference is declared). There is no additional, separate entity involved. That is reason enough.