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

Deprecation macro #969

Merged
merged 1 commit into from Oct 20, 2015

Conversation

Projects
None yet
6 participants
@Bromeon
Member

Bromeon commented Sep 19, 2015

Forum thread: http://en.sfml-dev.org/forums/index.php?topic=17888

I added a macro to deprecate functions and classes in a portable way. I used an object-style macro for simplicity and to avoid problems with commas in the evaluation. I don't consider a message necessary, as that's what the documentation is for.

Here is a minimal testing snippet that also shows the usage:

struct SFML_DEPRECATED MyClass
{
    SFML_DEPRECATED void memberFunc() {}
};

SFML_DEPRECATED void globalFunc() {}

int main() 
{
   MyClass().memberFunc();
   globalFunc();
}

The code also checks for a definition of the macro SFML_NO_DEPRECATED_WARNINGS. I thought about this, but I think it's not our job to enforce the warnings. If people don't want to see the warnings, that's their problem. Anyway, they will just turn off warnings in their compiler globally -- providing the option to selectively disable SFML's warnings is much better.

I tested the macro on Visual Studio 2015 and g++ 5.2.0 (MinGW). I would be glad about feedback for further compilers, compiler versions and platforms.

One problem with Visual Studio 2013 and 2015 is that the usage of deprecated symbols triggers a compiler error, not a warning by default. This can be changed with the /sdl- compiler flag, but it's something users explicitly have to do when working with SFML.

@Bromeon Bromeon added this to the 2.4 milestone Sep 19, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 19, 2015

Member

What about enums and enum members? There are also variables, but I don't think we'll need it.

Member

LaurentGomila commented Sep 19, 2015

What about enums and enum members? There are also variables, but I don't think we'll need it.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 19, 2015

Member

The Visual C++ compiler can deprecate a lot of different symbols using #pragma deprecated, including enums and enumerators. It's not portable, though.

In g++, the __attribute__((deprecated)) can be applied to enums. The documentation says it can also be used for enumerators, but such code doesn't compile at me.

I'll try to think of a solution to incorporate those two to have at least better coverage. The syntax may not be as beautiful, and I think we'll have to live with different levels of warnings depending on the compiler.

This SO thread shows several hacks to somehow achieve a similar behavior, but I don't think we should go in that direction. With C++14, the [[deprecated]] keyword will help -- but as far as I know, it can't be applied to enumerators, which is a shame.

Member

Bromeon commented Sep 19, 2015

The Visual C++ compiler can deprecate a lot of different symbols using #pragma deprecated, including enums and enumerators. It's not portable, though.

In g++, the __attribute__((deprecated)) can be applied to enums. The documentation says it can also be used for enumerators, but such code doesn't compile at me.

I'll try to think of a solution to incorporate those two to have at least better coverage. The syntax may not be as beautiful, and I think we'll have to live with different levels of warnings depending on the compiler.

This SO thread shows several hacks to somehow achieve a similar behavior, but I don't think we should go in that direction. With C++14, the [[deprecated]] keyword will help -- but as far as I know, it can't be applied to enumerators, which is a shame.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 19, 2015

Member

I posted on the forum something a while back about clang; I don't remember exactly what it was but it might help you.

Member

mantognini commented Sep 19, 2015

I posted on the forum something a while back about clang; I don't remember exactly what it was but it might help you.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 20, 2015

Member

Yes, theoretically it's the same for g++, but in practice the docs are wrong, and the usage leads to a syntax error. Another problem is, we have to insert a macro in different places:

// MSVC
enum __declspec(deprecated) Enum // <- allowed, but has no effect
{
    OldEnumerator,
    NewEnumerator,
};

#pragma deprecated(Enum)
#pragma deprecated(OldEnumerator)
// Clang, g++
enum __attribute__((deprecated)) Enum // <- works on both Clang and g++
{
    OldEnumerator __attribute__((deprecated)), // <- works only on Clang (apparently)
    NewEnumerator,
};

Furthermore, the MSVC #pragma is based on identifiers, not compile-time symbols, similar to the preprocessor. That is, every occurrence of that identifier, even if it refers to a different symbol (e.g. a class in a different namespace) will be mentioned in compiler warning. That's not really useful, except for macros.

To sum up, we can deprecate enums on g++/Clang, but not on MSVC. We cannot portably deprecate enumerators. I think we should resort to runtime warnings in that case.

I don't get why such things haven't been standardized in C++ from the beginning, something like Java's @Deprecated annotation is so simple and useful. C++14's [[deprecated]] is nice, but it will still take ages until we can portably use it... 😕

Member

Bromeon commented Sep 20, 2015

Yes, theoretically it's the same for g++, but in practice the docs are wrong, and the usage leads to a syntax error. Another problem is, we have to insert a macro in different places:

// MSVC
enum __declspec(deprecated) Enum // <- allowed, but has no effect
{
    OldEnumerator,
    NewEnumerator,
};

#pragma deprecated(Enum)
#pragma deprecated(OldEnumerator)
// Clang, g++
enum __attribute__((deprecated)) Enum // <- works on both Clang and g++
{
    OldEnumerator __attribute__((deprecated)), // <- works only on Clang (apparently)
    NewEnumerator,
};

Furthermore, the MSVC #pragma is based on identifiers, not compile-time symbols, similar to the preprocessor. That is, every occurrence of that identifier, even if it refers to a different symbol (e.g. a class in a different namespace) will be mentioned in compiler warning. That's not really useful, except for macros.

To sum up, we can deprecate enums on g++/Clang, but not on MSVC. We cannot portably deprecate enumerators. I think we should resort to runtime warnings in that case.

I don't get why such things haven't been standardized in C++ from the beginning, something like Java's @Deprecated annotation is so simple and useful. C++14's [[deprecated]] is nice, but it will still take ages until we can portably use it... 😕

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Sep 23, 2015

Contributor

I think we should provide a message with the deprecation warning. It will save us from a lot of forum post from users asking why there is a deprecation and what they should do about it. I am aware that the documentation is meant for that, but take a look at the doc of sf::Event. It's noted that sf::Event::mouseWheel is deprecated, but there is no notice what you should use instead. It's not very informative. Stuff like that will happen. It's safer to have another message printed when the usage occurs.

For the user is much nicer if he has a warning and a text explaining him how to fix it, instead of a warning only saying this is old and shouldn't be used anymore (or at least I have found that in the past using other libraries). To me that's like simply returning when there is an error and not printing an error message. Or throwing an exception with no error message and relying on the user to look up what this type of exception means.

Contributor

Foaly commented Sep 23, 2015

I think we should provide a message with the deprecation warning. It will save us from a lot of forum post from users asking why there is a deprecation and what they should do about it. I am aware that the documentation is meant for that, but take a look at the doc of sf::Event. It's noted that sf::Event::mouseWheel is deprecated, but there is no notice what you should use instead. It's not very informative. Stuff like that will happen. It's safer to have another message printed when the usage occurs.

For the user is much nicer if he has a warning and a text explaining him how to fix it, instead of a warning only saying this is old and shouldn't be used anymore (or at least I have found that in the past using other libraries). To me that's like simply returning when there is an error and not printing an error message. Or throwing an exception with no error message and relying on the user to look up what this type of exception means.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 23, 2015

Member

I don't think it is too much to ask, "xxx is deprecated, let's look at its documentation to know why and what to use instead". The documentation gives us more room to explain things properly.

And users will have to look up the documentation anyway, because they'll need to learn how to use the replacement API... We can't realistically count on the compiler message alone to inform the user about the modification.

Member

LaurentGomila commented Sep 23, 2015

I don't think it is too much to ask, "xxx is deprecated, let's look at its documentation to know why and what to use instead". The documentation gives us more room to explain things properly.

And users will have to look up the documentation anyway, because they'll need to learn how to use the replacement API... We can't realistically count on the compiler message alone to inform the user about the modification.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2015

Member

Can people on other systems and compilers test this? It would be nice to merge it soon, as several other PRs depend on this feature.

Member

Bromeon commented Sep 30, 2015

Can people on other systems and compilers test this? It would be nice to merge it soon, as several other PRs depend on this feature.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 30, 2015

Member

Clang's implementation works fine on OS X! 👌

main.cpp:14:5: warning: 'MyClass' is deprecated [-Wdeprecated-declarations]
    MyClass().memberFunc();
    ^
main.cpp:5:24: note: 'MyClass' has been explicitly marked deprecated here
struct SFML_DEPRECATED MyClass
                       ^
main.cpp:14:15: warning: 'memberFunc' is deprecated [-Wdeprecated-declarations]
    MyClass().memberFunc();
              ^
main.cpp:7:38: note: 'memberFunc' has been explicitly marked deprecated here
                SFML_DEPRECATED void memberFunc() {}
                                     ^
main.cpp:15:5: warning: 'globalFunc' is deprecated [-Wdeprecated-declarations]
    globalFunc();
    ^
main.cpp:10:22: note: 'globalFunc' has been explicitly marked deprecated here
SFML_DEPRECATED void globalFunc() {}
                     ^
Member

mantognini commented Sep 30, 2015

Clang's implementation works fine on OS X! 👌

main.cpp:14:5: warning: 'MyClass' is deprecated [-Wdeprecated-declarations]
    MyClass().memberFunc();
    ^
main.cpp:5:24: note: 'MyClass' has been explicitly marked deprecated here
struct SFML_DEPRECATED MyClass
                       ^
main.cpp:14:15: warning: 'memberFunc' is deprecated [-Wdeprecated-declarations]
    MyClass().memberFunc();
              ^
main.cpp:7:38: note: 'memberFunc' has been explicitly marked deprecated here
                SFML_DEPRECATED void memberFunc() {}
                                     ^
main.cpp:15:5: warning: 'globalFunc' is deprecated [-Wdeprecated-declarations]
    globalFunc();
    ^
main.cpp:10:22: note: 'globalFunc' has been explicitly marked deprecated here
SFML_DEPRECATED void globalFunc() {}
                     ^
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Oct 5, 2015

Contributor

I tested the code with gcc 4.8.2 on Ubuntu Linux. The output looks like this:

.../main.cpp||In function ‘int main()’:|
.../main.cpp|12|warning: ‘void MyClass::memberFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:5) [-Wdeprecated-declarations]|
.../main.cpp|13|warning: ‘void globalFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:8) [-Wdeprecated-declarations]|
.../main.cpp|13|warning: ‘void globalFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:8) [-Wdeprecated-declarations]|

So everything seems to work, although I'm not sure why the globalFunc() deprecation warning is printed twice.

I agree that the documentation leaves more room for explanation, but it seems odd to explicitly not use an extra form of informing the user. After all on of SFML design goals is simple usage. Even something as simple as "foo is deprecated, use bar instead. Please refer to the documentation for more information.", would help pointing the user in the right direction. I can just say, that from a users perspective I found it very helpful, if the library I was using told me how to fix my warnings.

A thing I was wondering about while reading the code: Why is #pragma message() used for the non supported warning? Wouldn't #warning or even #pragma waning() be better?

Contributor

Foaly commented Oct 5, 2015

I tested the code with gcc 4.8.2 on Ubuntu Linux. The output looks like this:

.../main.cpp||In function ‘int main()’:|
.../main.cpp|12|warning: ‘void MyClass::memberFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:5) [-Wdeprecated-declarations]|
.../main.cpp|13|warning: ‘void globalFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:8) [-Wdeprecated-declarations]|
.../main.cpp|13|warning: ‘void globalFunc()’ is deprecated (declared at /home/max/workspace/SFML-test/main.cpp:8) [-Wdeprecated-declarations]|

So everything seems to work, although I'm not sure why the globalFunc() deprecation warning is printed twice.

I agree that the documentation leaves more room for explanation, but it seems odd to explicitly not use an extra form of informing the user. After all on of SFML design goals is simple usage. Even something as simple as "foo is deprecated, use bar instead. Please refer to the documentation for more information.", would help pointing the user in the right direction. I can just say, that from a users perspective I found it very helpful, if the library I was using told me how to fix my warnings.

A thing I was wondering about while reading the code: Why is #pragma message() used for the non supported warning? Wouldn't #warning or even #pragma waning() be better?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 5, 2015

Member

#warning leads to compiler errors when unknown.
#pragma message seems to be widely supported, I'm not sure about #pragma warning. And where is the difference?

Member

Bromeon commented Oct 5, 2015

#warning leads to compiler errors when unknown.
#pragma message seems to be widely supported, I'm not sure about #pragma warning. And where is the difference?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 11, 2015

Member

Further feedback?

It would be nice to merge it soon, as several other PRs depend on this feature.

😀

Member

Bromeon commented Oct 11, 2015

Further feedback?

It would be nice to merge it soon, as several other PRs depend on this feature.

😀

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 11, 2015

Member

👍

Member

binary1248 commented Oct 11, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 14, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Oct 14, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r merged commit fcb05fb into master Oct 20, 2015

@eXpl0it3r eXpl0it3r deleted the feature/deprecation branch Oct 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment