Slice Posted February 29, 2020 Share Posted February 29, 2020 With commit https://github.com/CloverHackyColor/CloverBootloader/commit/2de3b849570daed52becd7c089e0b63cf3f5d5d0 we have a crash navigating menu Options. GCC53 compilation. Link to comment Share on other sites More sharing options...
Slice Posted February 29, 2020 Share Posted February 29, 2020 3 hours ago, apianti said: 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? Yes, in theory they are same. I tested int new_tests() { SETTINGS_DATA* A = new SETTINGS_DATA; SETTINGS_DATA* B = new SETTINGS_DATA(); AsciiStrCpyS(A->VendorName, 64, "Apple"); DebugLog(2, "new_tests() A:%a\n", A->VendorName); AsciiStrCpyS(B->VendorName, 64, A->VendorName); DebugLog(2, "new_tests() B:%a\n", B->VendorName); return 0; } And disassembled this part. In the both cases there is a call to new() with argument = 0xc10 which is sizeof(SETTINGS_DATA); Strange... Link to comment Share on other sites More sharing options...
Jief_Machak Posted February 29, 2020 Author Share Posted February 29, 2020 It's the same for type that has ctor. For Plain Old Data, struct with no constructor : it's not the same. I think it's worse : it can be the same... or not. C++ standard seems to say undefined behaviour for new T. Sorry, forgot where I read that. Your online compiler is zeroing memory even for new T (T is a POD), I've just tried. I think we can stick to AllocateZeroPool for old struct until we convert them to a class with a ctor. That way, we're sure what's happening under the hood. Link to comment Share on other sites More sharing options...
Slice Posted March 1, 2020 Share Posted March 1, 2020 You probably forgot to commit some codes. getBadgeImage etc. is not defined anywhere. Link to comment Share on other sites More sharing options...
Slice Posted March 1, 2020 Share Posted March 1, 2020 OK, I see new files. They were not included into search area. Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 1, 2020 Author Share Posted March 1, 2020 Defined if menu_items.h (this file will get smaller soon, I promise :-) ) Link to comment Share on other sites More sharing options...
Slice Posted March 2, 2020 Share Posted March 2, 2020 Wait please while I make XPointer class. 1 Link to comment Share on other sites More sharing options...
Slice Posted March 2, 2020 Share Posted March 2, 2020 Now I will wait your news. Link to comment Share on other sites More sharing options...
Slice Posted March 2, 2020 Share Posted March 2, 2020 @Jief_Machak It's a pity operator new() may produce memset() ; 153 : DuplicateEntry = new LOADER_ENTRY(); 00020 b9 e8 00 00 00 mov ecx, 232 ; 000000e8H 00025 e8 00 00 00 00 call ??2@YAPEAX_K@Z ; operator new 0002a 48 8b d8 mov rbx, rax 0002d 48 85 c0 test rax, rax 00030 74 45 je SHORT $LN5@DuplicateL 00032 33 d2 xor edx, edx 00034 41 b8 e8 00 00 00 mov r8d, 232 ; 000000e8H 0003a 48 8b c8 mov rcx, rax 0003d e8 00 00 00 00 call memset 00042 0f 57 c0 xorps xmm0, xmm0 How can we avoid it? Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 2, 2020 Author Share Posted March 2, 2020 Hi, I removed the pointer to XImage in XPointer. XPointer "owns" these 3 image objects ? Let's declare them as we would declare an int. delete and destructor not needed anymore, dtor will be called bt C++. I removed #include <Platform.h> from "XImage.h". We really can't do that, as Platform.h is including directly or indirectly these objects, leading to circular dependancy, which didn't compile once I remove the pointer to XImage (It was compiling before because C++ don't care to have the definition of the pointed object). Include Platform.h in .cpp is fine though. CheckMouseEvent has a REFIT_MENU_SCREEN as parameter. It uses and modify Screen->mAction. I always prefer to avoid modifying data from other. In a perfect world every method would take in (const) parameter and return some out parameter (and not use any globals). The caller can decide what to do the out parameters. I know it's too annoying to always do that, but we can get as close as possible. Could we use a local ACTION variable that we'll give back to the caller, so we don't modify the REFIT_MENU_SCREEN struct (soon to be an object, right :-)) ? Can we pass "Screen->Entries" as parameter instead of "REFIT_MENU_SCREEN *Screen", so we won't circular dependancies when the future object REFIT_MENU_SCREEN will use XPointer ? Link to comment Share on other sites More sharing options...
Slice Posted March 2, 2020 Share Posted March 2, 2020 I committed next large step. ScrollState is now part of REFIT_MENU_SCREEN. If we make RunMenu to be part of screen then it will simplify codes. XPointer now included into Screen. I too hurry and my current sources are far from complete. I committed them as next step. Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 2, 2020 Author Share Posted March 2, 2020 PS : sorry, I forgot to say : what you did was perfectly valid. Allocate in ctor, delete in dtor. That's enough to ensure no memory leak. But in my experience, using ptr is a bit more error prone. Either we share it with another object, or we destruct it outside of the object. By declaring the object (and reference for parameter and return value) instead of a pointer, we are less tempted to share the ptr. Or, if we do, it's intentional. That fit with my habit : pointer should be only used for OUT parameter of method (of course, that's not 100% doable, but close). Also, I tried to never use the operator '->'. I can't tell how many time I moved code and I had to switch from . to -> or viceversa. It's so painful. If I have to write a method with an out parameter, say void myMethod(SCREEN* scr). I'd declare myMethod(SCREEN* scrPtr), and first line would be SCREEN& scr = *scrPtr. If one day, after improvement, I realise I don't need to modify SCREEN anymore, I just change myMethod(SCREEN* scrPtr) to myMethod(const SCREEN& scr) (NOTE : I removed Ptr from the name) and remove the first line ! Link to comment Share on other sites More sharing options...
Slice Posted March 2, 2020 Share Posted March 2, 2020 I am learning continuously. Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 2, 2020 Author Share Posted March 2, 2020 As an example of : - use ptr for OUT or IN/OUT parameter (IN/OUT parameter should generally be avoided). - use const reference for IN parameter - "never" use const ptr : that's useless because we have reference. - never ever use non-const ref for parameter : they are really confusing ! - use non-const ref instead ptr inside a method body : that's for convenience and they cost nothing in size or speed (no code generated for T& a = *aPtr) I've committed a small change to WaitForInputEventPoll in IO.cpp. 2 Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 What will be the good c++ replacement for typedef VOID (*MENU_STYLE_FUNC)(IN UINTN Function, IN CONST CHAR16 *ParamText);? Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 3, 2020 Author Share Posted March 3, 2020 Oh yes, this. A slight design change. I think (but I may have to look closer), make a class, for example GraphicRendering, with a virtual function void myRenderingFunc(IN UINTN Function, IN CONST CHAR16 *ParamText). Then create different subclass (eg GraphicRenderingSubClass1, 2, etc.) with the same virtual function. Store a reference this way "const GraphicRendering& render." Then call render.myRenderingFunc(). The beauty of dynamic (or late) bindings instead of static (compile time) bindings. C++ can do both, depending if you declare the method virtual. The safe side : always declare your method virtual. That's why Java and Objective-C doesn't implement static bindings. I can help if you commit. I didn't touch the code much because I don't want to create too much conflicts (a refactoring process can touch a LOT of source files). Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 OK, I'll finish menu refactoring very soon and then commit. Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 I have committed. Lokk please multiple definitions errors like d:\projects\cloverbootloader\refit_uefi\libeg\../refit/IO.h(116): error C2720: 'REFIT_MENU_SCREEN::_PoolPrint': 'extern' storage-class specifier illegal on members d:\projects\cloverbootloader\refit_uefi\libeg\../refit/IO.h(122): error C2720: 'REFIT_MENU_SCREEN::_PPrint': 'extern' storage-class specifier illegal on members d:\projects\cloverbootloader\refit_uefi\platform\device_inject.h(99): error C2071: 'REFIT_MENU_SCREEN::nvdevice': illegal storage class d:\projects\cloverbootloader\refit_uefi\platform\device_inject.h(157): error C2071: 'REFIT_MENU_SCREEN::device_inject_string': illegal storage class d:\projects\cloverbootloader\refit_uefi\platform\device_inject.h(158): error C2071: 'REFIT_MENU_SCREEN::device_inject_stringdata': illegal storage class d:\projects\cloverbootloader\refit_uefi\platform\device_inject.h(159): error C2071: 'REFIT_MENU_SCREEN::device_inject_stringlength': illegal storage class d:\projects\cloverbootloader\refit_uefi\platform\kernel_patcher.h(75): error C2071: 'REFIT_MENU_SCREEN::KernelRelocBase': illegal storage class Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 3, 2020 Author Share Posted March 3, 2020 Yep, doing that in the next minute. Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 3, 2020 Author Share Posted March 3, 2020 Sorry, got delayed. I can't have the same error as you. I found line 400 of menu_items.h, there's a ',' at the end of the line instead of ';'. That can generate a lot of error. That fixed, IO.cpp is compiling fine. I also removed const in UpdateAnime (I think I've put it as a try). So main.cpp en screen.cpp is compiling. menu.cpp is not compiling because of the refactoring that is going on. Tell me which file doesn't compile on your side. Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 For this moment I stopped with functions *MenuStyle. If I keep them as C then they have no access to class member like screen. If I include them into the class them I can't include it as argument like RunGenericMenu(GraphicsMenuStyle, &Index, ChosenEntry); Make new classes for override REFIT_MENU_SCREEN? Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 It is just simpler to make an enum value and a function with switch... Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 3, 2020 Author Share Posted March 3, 2020 There is pointer to member function. I'm doing it now. Please wait few minutes. Link to comment Share on other sites More sharing options...
Jief_Machak Posted March 3, 2020 Author Share Posted March 3, 2020 OK, I've committed using pointer-to-member. Everything is compiling here. I didn't try VS. Let me know. Link to comment Share on other sites More sharing options...
Slice Posted March 3, 2020 Share Posted March 3, 2020 20 minutes ago, Jief_Machak said: OK, I've committed using pointer-to-member. Everything is compiling here. I didn't try VS. Let me know. I have conflicts with my current changes. Will resolve them a little later. Link to comment Share on other sites More sharing options...
Recommended Posts