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
2 hours ago, Jief_Machak said:

VectorGraphics, line 489 & 492 : SelectionImages still refer to the global EG_IMAGE *SelectionImages[6].

 

Maybe ParseSVGXTheme should become a method of XTheme, so SelectionImages would refer to the member ?

 

yes it is not thoughtfully to the end

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
On 3/25/2020 at 9:42 PM, Jief_Machak said:

I've switched to new printf format for DebugLog.

So now utf8 and utf16 works.  :

  %s for CHAR8* instead of %a

  %ls for CHAR16* (wchar_t*). Before, %s was only working if char code were < 128 (useless :-). It may not works in console because of console charset

  %x output hex letter as lower case. It wasn't the case before.

  %X works as usual.

  %lld for 64 bits.

 

If you use GCC or Xcode, there is warnings in case of mismatch.

See

~~~

- AsciiSPrint(NameCard, 32, "pci%x,%x\0", FakeVen, FakeID);

+ snprintf(NameCard, 32, "pci%X,%X", FakeVen, FakeID);

LowCase(NameCard);

~~~

 

Next line is to convert upper letters to lower case letters. But isn't it better to use %x format?

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

%x for Ascii..Print function prints in capital letter. To not change behaviour, I converted %x in %X. Just in case some other part of the code would parse this and assume it is capital letter.

But of course, if it's nicer in lowercase letter, you can put it back to %x.

I answered a bit fast. I did systematic replacement to not change behaviour. I didn't see that this was converted to lowercase afterward.

So definitely YES, we can print directly in lowercase and spare the LowCase(NameCard);

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
1 minute ago, Jief_Machak said:

%x for Ascii..Print function prints in capital letter. To not change behaviour, I converted %x in %X. Just in case some other part of the code would parse this and assume it is capital letter.

But of course, if it's nicer in lowercase letter, you can put it back to %x.

Yes, this is very special case where lower case is obligatory.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted

Why the procedure 

void XTheme::FillByEmbedded()

crashes?

4:841  0:002  XArray::ElementAt(xsize) -> Operator [] : index > m_lenA fatal error happened. System halted

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

Please, don't add any AllocateZeroPool(), the goal is to remove them ! (ex: legacy.cpp:151).

If you need to convert a String, use, for example in that case 

XString().takeValueFrom(Volume->LegacyOS->IconName)

If you need to give the result to a function that wants CHAR8* : 

XString().takeValueFrom(Volume->LegacyOS->IconName).c_str()

The other way around (utf8 to utf16) 

XStringW().takeValueFrom("blabla")
XStringW().takeValueFrom("blabla").wc_str()

The ideal would be to refactor the struct LEGACY_OS and replace "CONST CHAR16 *IconName;" by "XString IconName" or "XStringW IconName" (I think XString is enough).

But I totally agree not to refactor everything at the same time and these "takeValueFrom()" are precisely there for that : bridging old code with new.

Link to comment
Share on other sites
Pene
Pene
Posted
Posted (edited)

Hi Jief,


I'm getting these warnings lately: 

Active Platform          = CloverBootloader/Clover.dsc
...build: : warning: Module MetaFile [Sources] is missing local header!
    Local Header: cloverbootloader/refit_uefi/cpp_unit_test/printlib-test.h not found in CloverBootloader/rEFIt_UEFI/refit.inf
build: : warning: Module MetaFile [Sources] is missing local header!
    Local Header: cloverbootloader/refit_uefi/cpp_unit_test/printlib-test.h not found in CloverBootloader/rEFIt_UEFI/refit.inf
build: : warning: Module MetaFile [Sources] is missing local header!
    Local Header: cloverbootloader/refit_uefi/cpp_unit_test/poolprint-test.h not found in CloverBootloader/rEFIt_UEFI/refit.inf
build: : warning: Module MetaFile [Sources] is missing local header!
    Local Header: cloverbootloader/refit_uefi/cpp_unit_test/poolprint-test.h not found in CloverBootloader/rEFIt_UEFI/refit.inf
 done!

I added some missing files to refit.inf previously when similar warnings appeared, but I thought to bring it to your attention to look what actually should be in refit.inf :)

 

Edited by Pene
Link to comment
Share on other sites
Jief_Machak
Jief_Machak
Posted
  • Author
Posted

I know you're right.

Because I'm not using the edk build system (way too slow) for the dev, I usually do see these warning.

I compile once with edk build system before committing. And I don't always see the warnings because they are at the beginning. And sometimes it's quite late at night... :-)

 

My bad, I'll try to get the habit to add it to refint.inf as soon as I create a .h.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted
2 hours ago, Jief_Machak said:

Please, don't add any AllocateZeroPool(), the goal is to remove them ! (ex: legacy.cpp:151).

If you need to convert a String, use, for example in that case 


XString().takeValueFrom(Volume->LegacyOS->IconName)

If you need to give the result to a function that wants CHAR8* : 


XString().takeValueFrom(Volume->LegacyOS->IconName).c_str()

The other way around (utf8 to utf16) 


XStringW().takeValueFrom("blabla")
XStringW().takeValueFrom("blabla").wc_str()

The ideal would be to refactor the struct LEGACY_OS and replace "CONST CHAR16 *IconName;" by "XString IconName" or "XStringW IconName" (I think XString is enough).

But I totally agree not to refactor everything at the same time and these "takeValueFrom()" are precisely there for that : bridging old code with new.

What about this exercise? 

      CHAR16 *TmpArgs = NULL;
      if (AsciiStrLen(gSettings.BootArgs) > 0) {
        TmpArgs = PoolPrint(L"%a", gSettings.BootArgs);
      }
...
	gSettings.OptionsBits = EncodeOptions(TmpArgs);
...
      if (TmpArgs) {
        FreePool(TmpArgs);
        TmpArgs = NULL;
      }

It looks like can be smarter.

Link to comment
Share on other sites
Jief_Machak
Jief_Machak
Posted
  • Author
Posted 
      XStringW TmpArgs;
      if (AsciiStrLen(gSettings.BootArgs) > 0) {
        TmpArgs.SPrintf("%s", gSettings.BootArgs);
      }
...
	gSettings.OptionsBits = EncodeOptions(TmpArgs.wc_str());
...
//      if (TmpArgs) {
//        FreePool(TmpArgs);
//        TmpArgs = NULL;
//      }

Something like that ?

Link to comment
Share on other sites
Jief_Machak
Jief_Machak
Posted
  • Author
Posted
1 hour ago, Slice said:

Why not 


gSettings.OptionsBits = EncodeOptions(XStringW().takeValueFrom(gSettings.BootArgs).wc_str());

Will empty input string to be a problem?

Actually, it's not. I'm surprised but takeValueFrom call StrCpy that allows NULL value to be interpreted the same as an empty string.

I also saw that you modified XStringWP that way. I'm usually not a big fan of this, because it makes takeValueFrom(NULL) and takeValueFrom("") producing the same result. By experience, I can say that it sometime lead to undetected problems.

In the other hand, it makes thing easier during the refactoring...

 

A middle solution could be to have a new takeValueFromAllowNULL , so at least, we know it's intentional. I know you don't like long method names but there is 2 big advantages : it's 100% cleat what it does, and it can be tracked down easily.

Once we have converted all the strings, calls to method takeValueFrom must naturally disappeared. If not, it's easy to find them. 

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

I removed XStringWP. That was temporary anyway.

To create an XString from litteral, it's now "blabla"_XS or L"blabla"_XSW.

 

Easiest to initialise a XString :

XString str = 'blabla"_XS;

XString str = SPrintf(...); // with Return Value Optimization, only one allocation must take place. 

 

Same for wide

XStringW strw = 'blabla"_XSW;

XStringW strw = SWPrintf(...);// with Return Value Optimization, only one allocation must take place. 

 

Of course, "blabla"_XS can be given directly as a parameter to a method that accept const XString&

 

printf family is way more efficient than concatenation like "bla1"_XS + str + "bla2"_XS.

Link to comment
Share on other sites
Slice
Slice
Posted
Posted

It seems I encounter a problem with 

XImage  SelectionImages[6];

SelectionImages[0] is not the same I created. Looks like I have to unroll the array into separate images.

Link to comment
Share on other sites
Jief_Machak
Jief_Machak
Posted
  • Author
Posted
On 4/1/2020 at 6:04 PM, Slice said:

 // all_tests();

Looks like when the tests are working here, they are working with everyone. Couldn't be 100% sure at the beginning, that's why I put the test at the beginning of refit_main.

 

What would be great, is to make a separate efi with all the tests. Some courage anyone ?

Link to comment
Share on other sites
 Share
Go to topic listing
×
×
  • Create New...