Slice Posted February 27, 2020 Share Posted February 27, 2020 So the problem with new<->FreePool. Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 27, 2020 Author Share Posted February 27, 2020 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... Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 27, 2020 Author Share Posted February 27, 2020 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 ? Link to comment Share on other sites More sharing options...
Pene Posted February 27, 2020 Share Posted February 27, 2020 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 More sharing options...
Jief_Machak Posted February 27, 2020 Author Share Posted February 27, 2020 (edited) 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 Link to comment Share on other sites More sharing options...
Pene Posted February 27, 2020 Share Posted February 27, 2020 (edited) 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 Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 27, 2020 Author Share Posted February 27, 2020 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. 1 Link to comment Share on other sites More sharing options...
Pene Posted February 27, 2020 Share Posted February 27, 2020 And now it seems to work properly. It was hanging for me also when it was initialized with new. Link to comment Share on other sites More sharing options...
Slice Posted February 28, 2020 Share Posted February 28, 2020 Hi @Jief_Machak The line *Child = new (__typeof_am__(**Child)); should not be *Child = new (sizeof(**Child)); ? Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 28, 2020 Author Share Posted February 28, 2020 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 ? Link to comment Share on other sites More sharing options...
Slice Posted February 28, 2020 Share Posted February 28, 2020 Yes, commented. I want to make "new" working. Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 28, 2020 Author Share Posted February 28, 2020 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 More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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 More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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))(); Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 Well, new TYPE() and new TYPE is the same. I used in what I've refactored. Would that be a gcc/clang extension ? Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 I'm working on remove my new classes form lib.h. Link to comment Share on other sites More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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 More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 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. Link to comment Share on other sites More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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 More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 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. Link to comment Share on other sites More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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 More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 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 () !! 1 Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 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. Link to comment Share on other sites More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 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 More sharing options...
apianti Posted February 29, 2020 Share Posted February 29, 2020 (edited) 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 Link to comment Share on other sites More sharing options...
Recommended Posts