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

Suppress warnings in Function.cpp #550

Merged
merged 2 commits into from
Nov 5, 2015
Merged

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Nov 4, 2015

For backward compatibility, we have to call deprecated functions in Function.cpp, but the compilers complain for these calls. This pull request suppresses the warnings only for those calls. See also #544.

Note that the suppression code couldn't be made as macros. It needs to use compiler dependent #pragma directives but directives is not allowed to be declared in macros.

For backward compatibility, we have to call deprecated functions in Function.cpp, and compilers complain for these calls. This commit suppresses the warnings only for those calls. See also #544.

Note that the suppression code couldn't be made as macros. It needs to use compiler dependent #pragma directives but directives is not allowed to be decleared in macros.
@jslee02 jslee02 added this to the DART 5.1.1 milestone Nov 4, 2015
@mxgrey
Copy link
Member

mxgrey commented Nov 4, 2015

I wonder if we could make this more reusable, for example maybe we could have

#define DART_SUPPRESS_DEPRECATED_START // begin suppressing deprecated warnings, based on the compiler
#define DART_SUPPRESS_DEPRECATED_END // stop suppressing deprecated warnings, based on the compiler

And then we could wrap those around any function calls that we don't want to see deprecation warnings for.

@mkoval
Copy link
Collaborator

mkoval commented Nov 4, 2015

@mxgrey I don't think this is possible because of what @jslee02 said in his original post:

It needs to use compiler dependent #pragma directives but directives is not allowed to be declared in macros.

@mxgrey
Copy link
Member

mxgrey commented Nov 4, 2015

Right, I see. I guess the only way to overcome it would be to do some pre-preprocessing, but it's probably not worth it to take that route for this.

@mkoval
Copy link
Collaborator

mkoval commented Nov 4, 2015

Actually, that isn't quite true. According to StackOverflow you can use _Pragma("foo") as an equivalent to #pragma foo that can be used in macros.

[Edit] This appears to be an addition in C++11.

@jslee02
Copy link
Member Author

jslee02 commented Nov 4, 2015

@mxgrey Yes, I started with similar approach you suggested, but couldn't find a way to contain #pragma in macro. However, _Pragma(string) seems to be able to do it, and clang, gcc, and MSVC all support this operator. I will try with it now. Thanks @mkoval !

@jslee02
Copy link
Member Author

jslee02 commented Nov 4, 2015

Updated the warning suppression with macros using _Pragma operator.

[Edit] This appears to be an addition in C++11.

Hm, I need to check if MSVC supports this.

@mkoval
Copy link
Collaborator

mkoval commented Nov 4, 2015

As usual, MSVC is a pain in the ass. It uses __pragma(foo) instead of _Pragma("foo"): note the two underscores, case, and lack of quotes. I think you can wrap this in a macro like this:

#ifdef _MSC_VER
#define PRAGMA(x) __pragma(x)
#else
#define PRAGMA(x) _Pragma(#x)
#endif

@jslee02
Copy link
Member Author

jslee02 commented Nov 4, 2015

Right, I had to use __pragma for MSVC. Many times MSVC requires special treatment.

The wrapper looks good but I would prefer to use it when we found more use cases.

@mxgrey
Copy link
Member

mxgrey commented Nov 5, 2015

This looks really good now 👍

@jslee02
Copy link
Member Author

jslee02 commented Nov 5, 2015

Thanks! Merging now.

jslee02 added a commit that referenced this pull request Nov 5, 2015
@jslee02 jslee02 merged commit 8d003b1 into release-5.1 Nov 5, 2015
@jslee02 jslee02 deleted the function_warning_suppress branch November 6, 2015 03:06
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