Jump to content

C++ proposition


Jief_Machak
 Share

823 posts in this topic

Recommended Posts

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.

Link to comment
Share on other sites

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 by Jief_Machak
Link to comment
Share on other sites

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 by Pene
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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!

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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 () !!

  • Like 1
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

 Share

×
×
  • Create New...