appstream icon indicating copy to clipboard operation
appstream copied to clipboard

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

Open nicolasfella opened this issue 2 years ago • 8 comments

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

nicolasfella avatar Jun 23 '22 15:06 nicolasfella

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...

ximion avatar Jun 27 '22 20:06 ximion

In C++20 requires is a reserved keyword

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

hughsie avatar Jun 28 '22 08:06 hughsie

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.

ximion avatar Jun 28 '22 08:06 ximion

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

jwakely avatar Jun 28 '22 08:06 jwakely

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

marxin avatar Jun 28 '22 08:06 marxin

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 avatar Jun 28 '22 09:06 jwakely

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.

jwakely avatar Jun 28 '22 09:06 jwakely

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.

ximion avatar Aug 22 '22 11:08 ximion

Renamed it to requirements

nicolasfella avatar Nov 12 '22 01:11 nicolasfella

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?

nicolasfella avatar Nov 12 '22 01:11 nicolasfella

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.

jwakely avatar Nov 12 '22 01:11 jwakely