C++ proposition

[Slice]

So the problem with new<->FreePool.

[Jief_Machak]

To be confirm. new is supposed to be the same as AllocatePool (we defined the new operator), but I wouldn't be surprised there is something more...

[Jief_Machak]

Sorry, I'm not sure. What do you want to revert ?

@slice : Pene want to cancel the last 2 commits. I can make them disappear, but you will have to re-checkout.

Is that ok ?

[Pene]

PS, I think another reason that you may have problem with the 'new' is because it doesn't use AllocateZeroPool. And we assume in all kinds of cases that it's NULL without actually setting it to NULL.

I'm not sure new initializes it with zeros.

[Jief_Machak]

If you have current modification, just duplicate Clover folder and then use something like Araxis Merge to import them back.

 

3 minutes ago, Pene said:

PS, I think another reason that you may have problem with the 'new' is because it doesn't use AllocateZeroPool. And we assume in all kinds of cases that it's NULL without actually setting it to NULL.

I'm not sure new initializes it with zeros.

We could do that, because we supply the new implementation. But that's not the standard. But doing new on object will call the constructor, where you can set to zero.

SO it's probably better to start using new when we convert some struct to objetcs.

 

@Slice you ok, I'll go for the cancel of Pene commits ?

Edited February 27, 2020 by Jief_Machak

[Pene]

I think the merge appeared just because of a merge on my local tree. Now looking at it, it didn't actually change anything.

So apparently nothing should be canceled.

But how can we avoid those merge commits in the future?

I see it appeared also for example when Slice pushed, repeating the changes Jief committed before (https://github.com/CloverHackyColor/CloverBootloader/commit/448ee9568155af46393880a96cc37874e4fa8c70)

Edited February 27, 2020 by Pene

[Jief_Machak]

Yes, confirm all is good. There was 4 commits, just to change one line :-).

So @Pene you didn't commit a lot of changes. I agree, it's not always that clear, this "conflict resolution, merge commit".

Sign off, see you later.

[Pene]

And now it seems to work properly.

It was hanging for me also when it was initialized with new.

[Slice]

Hi @Jief_Machak

The line

*Child = new (__typeof_am__(**Child));

should not be

*Child = new (sizeof(**Child));

?

[Jief_Machak]

No. New take a type as parameter, not a size. I guess that "*Child = new (sizeof(**Child));" wouldn't compile.

I thought that line was commented out ?

[Slice]

Yes, commented. I want to make "new" working.

[Jief_Machak]

Pushed a big commit. There might be some bug, sorry.

 

Why "extern XObjArray<REFIT_VOLUME> Volumes;" appear now in a lot of different files ?

Well, we have a problem of circular dependancy.

 

Object in cpp_fundation should not need to include Plateform.h. So maybe we need to do a "PlatformBase.h" containing defintions for the need of cpp_fundation objects. Then Plateform.h can include the cpp_fundations objetcs so they'll be available to the whole project.

[Slice]

7 hours ago, Jief_Machak said:

Pushed a big commit. There might be some bug, sorry.

 

Why "extern XObjArray<REFIT_VOLUME> Volumes;" appear now in a lot of different files ?

Well, we have a problem of circular dependancy.

 

Object in cpp_fundation should not need to include Plateform.h. So maybe we need to do a "PlatformBase.h" containing defintions for the need of cpp_fundation objects. Then Plateform.h can include the cpp_fundations objetcs so they'll be available to the whole project.

Yes, Clover sources are very messy beginning from refit sources and growing monstrously.

Historically there was lib.h in refit. I created Platform.h with macOS definitions but next years the purpose of these files was confused.

clover-genconfig should link to actual version of Platform.h but compile it as C file. When I resolved this problem I got a compilation error like

/Users/sergey/src/CloverHackyColor/rEFIt_UEFI/entry_scan/common.cpp:505:31: error: 'Volumes' was not declared in this scope; did you mean 'ScanVolumes'?

  505 |       for (Index = 0; Index < Volumes.size(); ++Index) {

      |                               ^~~~~~~

      |                               ScanVolumes

It was defined in Platform.h and the file included into common.cpp. The fast way to resolve was to write extern ... in each module affected.

It will be better to split Platform.h to several files or move some definitions to lib.h.

 

I tested Clover after your "big big" commit and it works!

[Slice]

13 hours ago, Slice said:

Hi @Jief_Machak

The line

*Child = new (__typeof_am__(**Child));

should not be

*Child = new (sizeof(**Child));

?

There was a simple mistake:

*Child = new (__typeof_am__(**Child))();

[Jief_Machak]

Well, new TYPE() and new TYPE is the same. I used in what I've refactored. Would that be a gcc/clang extension ?

[Jief_Machak]

I'm working on remove my new classes form lib.h.

[Slice]

17 minutes ago, Jief_Machak said:

Well, new TYPE() and new TYPE is the same. I used in what I've refactored. Would that be a gcc/clang extension ?

The difference is Clover hangs or works. Confirmed with clang and with gcc.

[Jief_Machak]

In my new code. I have a lot of new TYPE. But TYPE are class. Maybe it works for classes.

Remember also that new is not supposed to zero the memory. Who pointed that to me ? Thanks. So new is NOT a replacement for AllocateZeroPool.

Careful.

[Slice]

AllocateZeroPool is slower and not always needed. It can be replaced by new + ZeroMem.

As well AllocateCopyPool can be replaced with new + CopyMem.

Can it be

A = new Type(0); instead of ZeroPool

and

B = new Type(A); instead of CopyCool? Assuming there are appropriate constructors.

[Jief_Machak]

we'll definitely do B = new Type(A); to copy objects. Even maybe do B = A.

In my experience, copying objects is rare, exception for basic types (XStringW).

 

For me it's A = new TYPE 

or

A = new TYPE().

 

Still don't understand why A = new TYPE crash.

[Slice]

30 minutes ago, Jief_Machak said:

we'll definitely do B = new Type(A); to copy objects. Even maybe do B = A.

In my experience, copying objects is rare, exception for basic types (XStringW).

 

For me it's A = new TYPE 

or

A = new TYPE().

 

Still don't understand why A = new TYPE crash.

We have to check disassembler to see what is the difference.

[Jief_Machak]

I learn something every day : there is a difference when doing new TYPE and new TYPE(), when TYPE is a plain old data object. Both are valid and doesn't mean the same thing in C++ (yahoo!).

The "new TYPE" is supposed to just alloc the memory and not initialize it. Good new is new TYPE() will zero init it, so it IS a replacement for AllocateZeroPool.

I checked with the real time debugger and when issuing "new TYPE", our custom new implementation is not called. I don't know if the compiler try to call malloc, and just silently fail (because it's not linked), but that explains it.

 

So a new rule : AllocateZeroPool is replaceable by new if we don't forget the () !!

[Jief_Machak]

When I compile with eclipse and clang : it runs. When I compile the ebuild.sh: it crashes. Guess where ? At "*Child = new (__typeof_am__(**Child))();"

I meant ebuild.sh using DEBUG_XCODE5.

Compiling with ebuild.sh using DEBUG_GCC53, works.

[Slice]

You may notice a file hebuild.sh. I created it to be most lazy.

I typed ".", "/", "h", "TAB" and "enter" to compile clover. It uses GCC53 by default.

I see no reason to support other compilers.

[apianti]

I'm not quite sure what you mean, new T should be equivalent to new T(). See here: https://onlinegdb.com/HyYKAbd4I.

 

EDIT: Did you check whether there are new operators being generated for each class that also need overridden?

Edited February 29, 2020 by apianti