Jump to content

C++ proposition

Jief_Machak

By Jief_Machak,
in Clover Development

 Share
823 posts in this topic

Recommended Posts

Slice
Slice
Posted
Posted
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
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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
Slice
Slice
Posted
Posted

@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
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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
Slice
Slice
Posted
Posted

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
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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.

 

 

  • Thanks 2
Link to comment
Share on other sites
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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
Slice
Slice
Posted
Posted

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
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

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
Slice
Slice
Posted
Posted

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
Slice
Slice
Posted
Posted
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
 Share
Go to topic listing
×
×
  • Create New...