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

Fix compilation with clang 13 #783

Merged
merged 1 commit into from Jan 9, 2022
Merged

Fix compilation with clang 13 #783

merged 1 commit into from Jan 9, 2022

Conversation

h0tc0d3
Copy link
Contributor

@h0tc0d3 h0tc0d3 commented Oct 22, 2021

Fix this issue with clang 13 #782

CMakeLists.txt Outdated
@@ -128,6 +128,7 @@ if(CMAKE_C_COMPILER_ID MATCHES "Clang")
-Wno-padded
-Wno-sign-conversion
-Wno-error=c11-extensions
-Wno-reserved-identifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The -Wno-reserved-identifier flag is not recognized by older versions of clang, such as clang 7.0.1 and 9.0.1. So this needs to be guarded by a feature detection test. I think we can model it after the HAVE_POISON_SYSTEM_DIRECTORIES_WARNING test:

    // Some glib-2.0 headers use identifier names that start with '_' followed by a capital letter.
    check_c_compiler_flag(-Wreserved-identifier HAVE_RESERVED_IDENTIFIER_WARNING)
    if(HAVE_RESERVED_IDENTIFIER_WARNING)
        add_definitions(-Wno-reserved-identifier)
    endif()

I have three other comments.

  1. /usr/include/ctype.h is part of the C implementation, so a C compiler should not warn that /usr/include/ctype.h uses reserved identifiers. This seems like a bug in clang 13. Indeed, I cannot reproduce the warnings about /usr/include/ctype.h with Clang 14.0.0 pre-release. I can, however, reproduce the warnings about glib-2.0 headers.

  2. It would be good to disable the -Wreserved-identifier warning only for glib-2.0 headers.

  3. This bug is an example of the problem with using clang's -Weverything flag: we are opting in to all the new compiler warning flags and it is a maintenance burden to update the -Wno-xxx list. It would be better to use -Wall -Wextra instead and add the other compiler warning flags that we find useful.

@h0tc0d3 h0tc0d3 changed the title Fix build with clang 13 Fix compilation with clang 13 Oct 27, 2021
@h0tc0d3
Copy link
Contributor Author

h0tc0d3 commented Oct 27, 2021

@wantehchang Thaks. Correct remarks. I updated pull request.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

My apologies for the late review. I am out of the office.

CMakeLists.txt Outdated
@@ -129,6 +129,13 @@ if(CMAKE_C_COMPILER_ID MATCHES "Clang")
-Wno-sign-conversion
-Wno-error=c11-extensions
)
# Some headers such as glib-2.0 use identifier names that start with '_' followed by a capital letter.
# The C standard reserves names beginning with an underscore and various other combinations.
# ISO/IEC 9899:1999 (aka C99 standard) §7.1.3 Reserved identifiers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change the '§' character to "Section" or "Sec."? We are trying to use only ASCII characters in the source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I removed '§' character.
I think it will be better.

Some headers such as glib-2.0 use identifier names that start with '_' followed by a capital letter.
The C standard reserves names beginning with an underscore and various other combinations.
ISO/IEC 9899:1999 (aka C99 standard) 7.1.3 Reserved identifiers
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@wantehchang
Copy link
Collaborator

I am very sorry that I forgot to merge this pull request. I will do that now.

@wantehchang wantehchang merged commit 9275660 into AOMediaCodec:master Jan 9, 2022
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.

None yet

2 participants