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

[qt] Rename Component::requires to avoid clash with C++20 #414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicolasfella
Copy link
Contributor

In C++20 requires is a reserved keyword

This results in a build failure when a C++20 project includes component.h

To avoid the issue the method gets a new name (requiresComponents). The old method is marked as deprecated and hidden when building in C++20 mode

Fixes #342

qt/component.cpp Outdated
}
#endif

QList<Relation> Component::requiresComponents() const
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO could be requiredComponents

@@ -364,7 +364,14 @@ QList<Relation> Component::recommends() const
return res;
}

#if __cplusplus < 202002L
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a #warning in an #endif so that if AppStreamQt is built with C++20, it will be ABI incompatible?

@ximion
Copy link
Owner

ximion commented Jun 27, 2022

Thank you for the patch! I do think requiresComponents is a terrible name though, because pretty much anything can be required, including screen dimensions, memory, mime-handlers or binaries, and not just components. So that name would be very misleading.
This patch would also make the library break ABI depending on which C++ standard it was compiled with, which I am not really happy with...

@hughsie
Copy link
Collaborator

hughsie commented Jun 28, 2022

In C++20 requires is a reserved keyword

That kind of change seems somewhat, well, hostile to your existing userbase...

@ximion
Copy link
Owner

ximion commented Jun 28, 2022

In C++20 requires is a reserved keyword

That kind of change seems somewhat, well, hostile to your existing userbase...

Yeah, it really sucks, it basically forces an ABI break - and some better-name searching (if requires_ isn't used, which looks ugly). People will absolutely want to use C++20 with this project though, so we do need to deal with.

@jwakely
Copy link

jwakely commented Jun 28, 2022

You can use asm to create a symbol alias to continue exporting a symbol with the old name, so there's no ABI break.

@marxin
Copy link

marxin commented Jun 28, 2022

@jwakely
Copy link

jwakely commented Jun 28, 2022

That kind of change seems somewhat, well, hostile to your existing userbase...

The requires keyword was first added to the C++ working draft in 2008, and although it was removed again before C++11 was published, the fact that Concepts were coming back at some point was always certain. It was a poor choice of identifier any time after 2008.

@jwakely
Copy link

jwakely commented Jun 28, 2022

I do think requiresComponents is a terrible name though, because pretty much anything can be required, including screen dimensions, memory, mime-handlers or binaries, and not just components. So that name would be very misleading.

Wouldn't requirements be better anyway? "requires" is a verb, and IIUC this function doesn't do something, it's a query for what is required.

@ximion
Copy link
Owner

ximion commented Aug 22, 2022

Wouldn't requirements be better anyway?

That's probably what I would go with... It's called "requires" because that's what the tag name is in metadata and it jives well with the existing replaces/extends/recommends/supports - to be consistent those would also need to be named "recommendations", "replacement_for", "supporting" etc, which is a bit weird.
I think requirements is fine here even though it breaks consistency, just to get rid of the name clash with "requires" becoming a reserved word.

In C++20 requires is a reserved keyword

This results in a build failure when a C++20 project includes component.h

To avoid the issue the method gets a new name (requirements). The old method is marked as deprecated and hidden when building in C++20 mode

Fixes ximion#342
@nicolasfella
Copy link
Contributor Author

Renamed it to requirements

@nicolasfella
Copy link
Contributor Author

https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes seek for alias ("target")

I'm not sure I follow. That site says "The alias attribute causes the declaration to be emitted as an alias for another symbol, which must have been previously declared with the same type". So in order to create an alias for it I have to declare it, but I can't, because the name is a keyword. Or am I misunderstanding something?

@jwakely
Copy link

jwakely commented Nov 12, 2022

You can define it in a translation unit compiled with C++17 (or earlier) where it's not a keyword, and make it an alias to the new name. Or just make it call the new function.

You can also declare it as an extern "C" symbol using its mangled name and alias that the the new mangled name, but that's less portable, and sneaky.

ximion added a commit that referenced this pull request Nov 27, 2022
The "requires" word became reserved in recent C++ standards, so we can
no longer use it here and it will be removed in AppStream 1.0.
We provide the replacement "requirements" function now, to simplify
future migrations.

CC: #414
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.

Build error in C++20 project
6 participants