C++ proposition

[Jief_Machak]

Now Version.h is only included from one place : Settings.cpp

[Slice]

4 hours ago, Jief_Machak said:

I'm on it.

First : which Version.h is the right one? Some files includes the one in the main dir and main.cpp include the one in rEFIt_UEFI.

It depends on build script. Let us suppose the good version is in root folder and correct all around this.

[Slice]

6 hours ago, Jief_Machak said:

Now, Platform.h is a meta-header that includes basic need for all modules. It's supposed to contains only includes.

DO NOT define anything in there. I know it seems handy because it's instantaneously shared with all, but please don't. If it make sense to share something with all modules (macro, extern var or function), define it in the module it belongs to and create an include in Platform.h.

 

The point is to have modules that makes sense. A module is a bit like a C++ object. Everything is defined in the couple .h .cpp, then other modules include the header and start using it.

 

Everyone's ok with that ?

oops, forgot to update refit. Wait a sec before pulling.

It is good but the separate project "Clover.app" want to see SETTINGS_DATA and so #include "Platform.h".

Now it should include Settings.h?

[Jief_Machak]

I have the memory of a post saying that Clover.app has now it's own copy SETTINGS_DATA.

It would be better if it's shared, of course. We can make a SettingsDataShared.h to share just what's needed and so avoid compilation problem.

For sure Clover.app shouldn't include Platform.h (but I understand why it did).

[Jief_Machak]

For now, if including the whole Settings.h works, why not...

Edited April 16, 2020 by Jief_Machak

[Jief_Machak]

Ok, so there is a problem when printing a unsigned value (%u, %x or %X) with a var type small then the formet : ie %X and UINT8. Because the uint8 is promoted to a bigger type, that's why we got a lot of 'F' before.

 

So we have to use the right specifier. One h for short, 2 hh for char and uint8.

 

The thing is that there is no warning if you use a smaller var than the format specifier when it's a 8 or 16 bits values. But there is a warning if you use %hhx with a 16 bits value.

 

Story short : when unsure of the type, start with hh and let the compiler tell you what

s right.

[Jief_Machak]

PS : I checked all the %u %x and %X format, I think.

[Slice]

@Jief_Machak

As long and long long are the same for C++ in macOS may be you can enable to use format %ld for INTN as it insistently proposed by XCode?

[Slice]

To illustrate

[Jief_Machak]

Even though long and long long are the same size, they are not the same type. It's annoying, I know.

So if INTN is defined as long long, we need the double l in %lld.

It's something builtin the compiler. It can be changed. I did it with GCC for Arduino. But the GCC source has to be patch and recompile...

[Slice]

We may change ProcessorBind.h definitions

  /// 8-byte unsigned value

  ///

  typedef unsigned long long  UINT64;

  ///

  /// 8-byte signed value

  ///

  typedef long long           INT64;

  ///

 

 

 

to be

/// 8-byte unsigned value

  ///

  typedef unsigned long   UINT64;

  ///

  /// 8-byte signed value

  ///

  typedef long            INT64;

  ///

 

Why not?

[Jief_Machak]

Entirely possible. But only do that if not Microsoft. long is 32 bits if compiled by VS. Which probably won't match to what EFI would expect.

If you'd like to change INT64, probably best to change UINT64 too.

 

Oh, but there is a problem if you do that : INT64 would %ld on CLANG/GCC and %lld on Microsoft. So I think we can't do that.

[Slice]

I did quick test

    DBG("void=%ld int=%ld long=%ld longlong=%ld enum=%ld\n",

        sizeof(void), sizeof(int), sizeof(long int), sizeof(long long), sizeof(EFI_ALLOCATE_TYPE));

The result is

11:305  0:000  void=1 int=4 long=8 longlong=8 enum=4

Expected?

11 minutes ago, Jief_Machak said:

Entirely possible. But only do that if not Microsoft. long is 32 bits if compiled by VS. Which probably won't match to what EFI would expect.

If you'd like to change INT64, probably best to change UINT64 too.

 

Oh, but there is a problem if you do that : INT64 would %ld on CLANG/GCC and %lld on Microsoft. So I think we can't do that.

But %lld is under your control. You may just change it interpretation for the MSC case.

My displeasure is just text editing in Xcode as I shown in the screen above. These warnings are very annoying.

[Jief_Machak]

Unfortunately no, the printf-like format checking is totally builtin compilers.

Yes, but in XCode, just click fix and they're gone.

Moreover : you have to click fix. It's a mistake to use %ld with long long and vice-versa.

 

Clover is compiling with 0 warnings on my computer. Isn't the same on yours ?

3 minutes ago, Slice said:

1:305  0:000  void=1 int=4 long=8 longlong=8 enum=4

Yes, expected.

and void=1 int=4 long=4 longlong=8 enum=4 for microsoft compilers.

[Slice]

Our build_rules set -Werror so Clover must be compiled without warning. But XCode is annoying.

Button "fix" will change %lld to %ld which leads to error in batch compilation.

Why XCode thinks that INTN is long?

It doesn't see headers?

[Jief_Machak]

??? Change %lld to %ld ??? Very strange. Mine does the opposite.

 

Can you try with my CloverX64 project ? I'm pretty sure XCode indexer is picking up the wrong ProcessorBind.h and thinks that int64 are long. But they are long long ! (line 189 in X64/ProcessorBind.h)

 

I wouldn't have spent so much time doing that if it were to have warnings.

[Jief_Machak]

It sees header,  but there is 9 different ProcessorBind.h in EDK. So maybe Xcode get confused.

 

Try with CloverX64 to confirm that you don't have an xcode problem. There should be no warnings at all.

Is that right ?

[Slice]

CloverX64 project is not analysed by Xcode because headers absent. How can it knows what is INTN? The definition is out of the project.

24 minutes ago, Jief_Machak said:

It sees header,  but there is 9 different ProcessorBind.h in EDK. So maybe Xcode get confused.

 

Try with CloverX64 to confirm that you don't have an xcode problem. There should be no warnings at all.

Is that right ?

I may exclude other ProcessorBind files from the project.

[Jief_Machak]

Just now, Slice said:

CloverX64 project is not analysed by Xcode because headers absent. How can it knows what is INTN? The definition is out of the project.

Not sure what that means. I didn't put the header in the project, right, but Xcode still finds them. Try do jump definition and you'll see it works (at least here it works). If I do jump definition on INT64, I end up in the correct ProcessorBind.h.

If I put a wrong printf format and I get the right warning..

Well, can you confirm it works.

[Slice]

44 minutes ago, Jief_Machak said:

 

Yes, expected.

and void=1 int=4 long=4 longlong=8 enum=4 for microsoft compilers.

Not expected. Xcode8 compilation stopped

/Users/sergey/src/Clover/rEFIt_UEFI/refit/main.cpp:2153:9: error: invalid application of 'sizeof' to an incomplete type 'void'
        sizeof(void), sizeof(int), sizeof(long int), sizeof(long long), sizeof(EFI_ALLOCATE_TYPE));
        ^     ~~~~~~

 

[Jief_Machak]

You're not supposed to take "sizeof(void)".

 

Which Xcode Scheme do you use when compiling with Clover.xcworkspace ?

 

[Slice]

4 minutes ago, Jief_Machak said:

Not sure what that means. I didn't put the header in the project, right, but Xcode still finds them. Try do jump definition and you'll see it works (at least here it works). If I do jump definition on INT64, I end up in the correct ProcessorBind.h.

If I put a wrong printf format and I get the right warning..

Well, can you confirm it works.

No. I clicked Jump to definition on INTN and see

This is the file MdePkg/Include/Ebc/ProcessorBind.h which is wrong.

4 minutes ago, Jief_Machak said:

You're not supposed to take "sizeof(void)".

 

Which Xcode Scheme do you use when compiling with Clover.xcworkspace ?

 

Default one

[Jief_Machak]

You did that in CloverX64 project ?

[Slice]

How to compile mtoc.NEW_jief?

iMac2017:Clover sergey$ ./buildmtoc.sh 
WORKSPACE must be defined to Clover root path
iMac2017:Clover sergey$ 

 

[Jief_Machak]

me :

and then, If I do it again