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

Revert "Put AVIF_NODISCARD after AVIF_API" again #2298

Conversation

wantehchang
Copy link
Collaborator

@wantehchang wantehchang commented Jul 18, 2024

This reverts commit da268fb.

I tested this with Visual Studio 2022 Version 17.10.3. It does not work. I must have made a mistake when I tested this with Visual Studio last time.

I found two webpages that support my test results:
https://stackoverflow.com/questions/67502217/warning-c5240-nodiscard-attribute-is-ignored-in-this-syntactic-position microsoft/STL#1702

I think this issue is settled.

This reverts commit da268fb.

I tested this with Visual Studio 2022 Version 17.10.3. It does not work.
I must have made a mistake when I tested this with Visual Studio last
time.

I found two webpages that support my test results:
https://stackoverflow.com/questions/67502217/warning-c5240-nodiscard-attribute-is-ignored-in-this-syntactic-position
microsoft/STL#1702

I think this issue is settled.
@wantehchang
Copy link
Collaborator Author

Here is my latest test of a standalone file.

The compiler version:

C:\src>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

The bad code:

C:\src>type bad.c
__declspec(dllimport) [[nodiscard]] int Add(int x, int y);

__declspec(dllexport) [[nodiscard]] int Double(int x) {
  return Add(x, x);
}
C:\src>cl /std:clatest /W4 /c bad.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

bad.c
bad.c(1): error C2059: syntax error: 'type'
bad.c(3): error C2059: syntax error: 'type'

The good code:

C:\src>type good.c
[[nodiscard]] __declspec(dllimport) int Add(int x, int y);

[[nodiscard]] __declspec(dllexport) int Double(int x) {
  return Add(x, x);
}
C:\src>cl /std:clatest /W4 /c good.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33811 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

good.c

@wantehchang wantehchang requested review from jzern and vrabaud July 20, 2024 15:51
@wantehchang
Copy link
Collaborator Author

I forgot to request code review. PTAL. Thanks!

@vrabaud
Copy link
Collaborator

vrabaud commented Jul 22, 2024

How come the CI does not catch it?

@wantehchang
Copy link
Collaborator Author

To catch the compilation error (either MSVC or clang-cl), we need a shared library build (-DBUILD_SHARED_LIBS=ON).

There are some two build errors in our shared library build. One is easy to fix (two gtest-based tests need to link with theavif_internal library). The other is a strange linker unresolved symbol error that I cannot reproduce locally. I haven't figured out what caused it, but one of my experiments worked, so at least we have a workaround. (I call it a workaround rarher than a fix because I cannot explain why it works.)

Finally, we cannot run the tests in a shared library build on Windows because the tests and avif.dll are in different directories, so the tests cannot find avif.dll. We should be able to fix this by setting the Path envirnonment variable before running the tests, or by copying avif.dll to the tests directory.

@wantehchang wantehchang merged commit 03acd65 into AOMediaCodec:main Jul 22, 2024
14 checks passed
@wantehchang wantehchang deleted the revert-avif-api-avif-nodiscard-reorder branch July 22, 2024 14:27
Copy link
Collaborator

@jzern jzern left a comment

Choose a reason for hiding this comment

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

lgtm

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.

3 participants