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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

AK: Remove workarounds for missing P0960R3 support in Clang #24345

Merged
merged 1 commit into from
May 21, 2024

Conversation

DanShaders
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 17, 2024
@ADKaster
Copy link
Member

The all red CI tells me that in fact, P0690R3 support in clang is not without bugs :D

@DanShaders DanShaders marked this pull request as draft May 17, 2024 04:06
@github-actions github-actions bot removed the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 17, 2024
@stelar7
Copy link
Contributor

stelar7 commented May 17, 2024

Have the oss-fuzz images been updated now? (ref: #22264)

@DanShaders
Copy link
Contributor Author

Should be, oss-fuzz is currently at Clang 17+\varepsilon version.

@DanShaders DanShaders marked this pull request as ready for review May 17, 2024 18:20
@github-actions github-actions bot added the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 17, 2024
@BertalanD
Copy link
Member

Have you checked if the latest Xcode can cope with this?

@DanShaders
Copy link
Contributor Author

No, I don't have access to MacOS

@nico
Copy link
Contributor

nico commented May 18, 2024

I patched this in and ran Meta/serenity.sh build lagom image on a mac with Xcode 15.2. That's the newest version that runs on macOS 13.5+ (I haven't installed macOS 14 yet), but not the very newest version (which is 15.4).

It failed like so:

[153/283] Building CXX object Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/TinyVGLoader.cpp.o
FAILED: Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/TinyVGLoader.cpp.o 
/Users/thakis/Downloads/ccache-4.9-darwin/ccache /usr/bin/clang++ -DENABLE_COMPILETIME_FORMAT_CHECK -DLibGfx_EXPORTS -I/Users/thakis/src/serenity/Meta/Lagom/../.. -I/Users/thakis/src/serenity/Meta/Lagom/../../Userland -I/Users/thakis/src/serenity/Meta/Lagom/../../Userland/Libraries -I/Users/thakis/src/serenity/Meta/Lagom/../../Userland/Services -I/Users/thakis/src/serenity/Build/lagom -I/Users/thakis/src/serenity/Build/lagom/Userland/Libraries -I/Users/thakis/src/serenity/Build/lagom/Userland/Services -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk -mmacosx-version-min=13.5 -fPIC -fcolor-diagnostics -Wall -Wextra -Wno-invalid-offsetof -Wno-unknown-warning-option -Wno-unused-command-line-argument -fno-exceptions -ffp-contract=off -Werror -fconstexpr-steps=16777216 -Wno-implicit-const-int-float-conversion -Wno-user-defined-literals -Wno-vla-cxx-extension -Wno-maybe-uninitialized -Wno-shorten-64-to-32 -fsigned-char -ggnu-pubnames -fPIC -O2 -g1 -Wno-overloaded-virtual -Wno-unused-private-field -std=c++2b -MD -MT Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/TinyVGLoader.cpp.o -MF Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/TinyVGLoader.cpp.o.d -o Userland/Libraries/LibGfx/CMakeFiles/LibGfx.dir/ImageFormats/TinyVGLoader.cpp.o -c /Users/thakis/src/serenity/Userland/Libraries/LibGfx/ImageFormats/TinyVGLoader.cpp
/Users/thakis/src/serenity/Userland/Libraries/LibGfx/ImageFormats/TinyVGLoader.cpp:549:19: error: no matching function for call to 'make'
    : m_context { make<TinyVGLoadingContext>(FixedMemoryStream { bytes }) }
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/thakis/src/serenity/Meta/Lagom/../../AK/NonnullOwnPtr.h:148:63: note: candidate template ignored: constraints not satisfied [with T = Gfx::TinyVGLoadingContext, Args = <AK::FixedMemoryStream>]
requires(IsConstructible<T, Args...>) inline NonnullOwnPtr<T> make(Args&&... args)
                                                              ^
/Users/thakis/src/serenity/Meta/Lagom/../../AK/NonnullOwnPtr.h:148:10: note: because 'IsConstructible<Gfx::TinyVGLoadingContext, AK::FixedMemoryStream>' evaluated to false
requires(IsConstructible<T, Args...>) inline NonnullOwnPtr<T> make(Args&&... args)
         ^
1 error generated.

@DanShaders
Copy link
Contributor Author

DanShaders commented May 18, 2024

Which llvm version is your Apple Clang based on?

Edit: oh, cppreference says that P0960 isn't supported on Apple Clang at all. Let's then ifdef out these definition on everything except Apple Clang. (I really want to delete these because without them make gives an error inline in clangd)

@DanShaders DanShaders closed this May 18, 2024
@DanShaders DanShaders deleted the checked-make branch May 18, 2024 22:30
@DanShaders DanShaders restored the checked-make branch May 18, 2024 22:30
@github-actions github-actions bot removed the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
@DanShaders DanShaders reopened this May 18, 2024
@github-actions github-actions bot added the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
With this change, ".*make.*" function family now does error checking
earlier, which improves experience while using clangd. Note that the
change also make them instantiate classes a bit more eagerly, so in
LibVideo/PlaybackManager, we have to first define SeekingStateHandler
and only then make() it.

Co-Authored-By: stelar7 <dudedbz@gmail.com>
@BertalanD
Copy link
Member

BertalanD commented May 21, 2024

I really want to delete these because without them make gives an error inline in clangd

Do you mean that the currently present workaround causes an issue with your clangd config? In that case, let's merge this. Otherwise, I don't see a point in #ifdef-ing out the code right now.

@DanShaders
Copy link
Contributor Author

It doesn't really work around any issues, just makes life easier, i. e.:

Before:
image
+ some long error when compiling

After:
image

Copy link
Member

@BertalanD BertalanD left a comment

Choose a reason for hiding this comment

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

Sounds like a good justification to me, LGTM!

We'll revisit this when the next Xcode major version comes out (late August or September, I presume).

@BertalanD BertalanD merged commit 38b51b7 into SerenityOS:master May 21, 2024
12 checks passed
@github-actions github-actions bot removed the 馃憖 pr-needs-review PR needs review from a maintainer or community member label May 21, 2024
@DanShaders DanShaders deleted the checked-make branch May 21, 2024 17:28
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

5 participants