Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernise edk2-platforms (replace EFI_D_* and __FUNCTION__ with DEBUG_* and __func__) #184

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2024

Update code to be more C11 compliant by using __func__

__FUNCTION__ is a pre-standard extension that gcc and Visual C++ among
others support, while __func__ was standardized in C99.

Since it's more standard, replace __FUNCTION__ with __func__ throughout
edk2-platforms.


Replace deprecated EFI_D_* with DEBUG_*

Replaced the deprecated EFI_D_{INFO,WARN,ERROR,VERBOSE} usage with
DEBUG_{INFO,WARN,ERROR,VERBOSE}.

Replaced the deprecated EFI_D_{INFO,WARN,ERROR,VERBOSE} usage with
DEBUG_{INFO,WARN,ERROR,VERBOSE}.

Signed-off-by: Rebecca Cran <[email protected]>
__FUNCTION__ is a pre-standard extension that gcc and Visual C++ among
others support, while __func__ was standardized in C99.

Since it's more standard, replace __FUNCTION__ with __func__ throughout
edk2-platforms.

Signed-off-by: Rebecca Cran <[email protected]>
Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Platform/ARM/*
Reviewed-by: Sami Mujawar [email protected]

Copy link
Member

@nhivp nhivp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Silicon/Ampere and Platform/Ampere, looks good to me. Thanks!

Copy link
Member

@nate-desimone nate-desimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for Platform/Intel & Silicon/Intel

Thanks @bcran!

@ghost
Copy link
Author

ghost commented Sep 6, 2024

Waiting on code owner review from @JohnAZoidberg, @ardbiesheuvel, @hrw, @kilaterlee, @leiflindholm, @lixianglai, @ndhillonm, @niruiyu, @vlsunil, @wojtas-marcin, and/or 1 others

@hrw
Copy link
Contributor

hrw commented Sep 9, 2024

For */Qemu/SbsaQemu:

Reviewed-by: Marcin Juszkiewicz [email protected]

@leiflindholm leiflindholm self-requested a review September 10, 2024 16:56
Copy link
Member

@leiflindholm leiflindholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you need to wait for more approvals for this non-functional change.

@ghost
Copy link
Author

ghost commented Sep 10, 2024

I'm not sure you need to wait for more approvals for this non-functional change.

@mdkinney It looks like merging is blocked waiting for additional reviews.

Waiting on code owner review from JohnAZoidberg, kilaterlee, lixianglai, niruiyu, and/or vlsunil.

Merging is blocked
Merging can be performed automatically with 1 approving review.

@mdkinney
Copy link
Member

mdkinney commented Sep 10, 2024

A CODEOWNERS file is present, but I do not see any code owner tags in this review. Is the CODEOWNERS file consistent? Perhaps it is being ignored due to missing contributors or files not covered.

I just noticed detailed status. There are missing reviews by required CODEOWNERS for the files modified by this PR. This is a large PR that touches 300+ files, and requirement of enabling CODEOWNERS is that all code owners must approve.

If this is hard to do in a single PR, then perhaps consider breaking up into multiple PR.

Also, with so many files modified in many packages, why was this not split up into more than 2 commits?

@mdkinney
Copy link
Member

mdkinney commented Sep 10, 2024

There is also something not right about the CODEOWNERS config in this repo. There should be an icon next to each assign reviewer that is a CODEOWNER. See this PR as an example: tianocore/edk2-codereview#91

There are also github actions to verify the consistency of a CODEOWNERS file.

Some example uses of this checker in:

If there are CODEOWNERS that have not joined the EDK II Branch Maintainers team, then that need to be fixed first. They either need to join or they need to be removed from CODEOWNERS.

@ghost
Copy link
Author

ghost commented Sep 10, 2024

If there are CODEOWNERS that have not joined the EDK II Branch Maintainers team, then that need to be fixed first. They either need to join or they need to be removed from CODEOWNERS.

It looks like Jeremy Soller (@jackpot51) hasn't accepted the invite yet, nor has Bibo Mao (@bibo-mao).

@hrw hrw merged commit 6cfae9d into tianocore:master Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.