-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
STYLE: Drop Visual C++ 2013 compiler support from itktiff/libtiff #4965
STYLE: Drop Visual C++ 2013 compiler support from itktiff/libtiff #4965
Conversation
ITK already dropped Visual C++ 2013 in 2018, with commit 0067eee "COMP: Require MSVC VS14 2015 for C++11 compatibility", by Bradley Lowekamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will be gone with the new version of libtiff. Please don't commit a conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @blowekamp Let's focus our efforts on updating the upstream version where this as already been addressed.
/* MSVC does not support C99 inline, so just make the inline keyword | ||
disappear for C. */ | ||
#ifndef __cplusplus | ||
# if defined(_MSC_VER) && _MSC_VER < 1900 | ||
# define inline | ||
# endif | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hjmjohnson @blowekamp but can you please explain? What is the upstream version of "tif_config.h.in"? Is it https://gitlab.com/libtiff/libtiff/-/blob/e8233c42f2e0a0ea7260c3cc7ebbaec8e5cb5e07/libtiff/tif_config.h.in ?
I'm asking because https://gitlab.com/libtiff/libtiff/-/blob/e8233c42f2e0a0ea7260c3cc7ebbaec8e5cb5e07/libtiff/tif_config.h.in does not have any _MSC_VER
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR can not be merged because it will conflict #4961. Updating sub-tree merges is very manual and time consuming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowekamp Interesting Your PR #4961 entirely removes "tif_config.h.in", right? Looking at https://github.com/InsightSoftwareConsortium/ITK/pull/4961/files So then, will the _MSC_VER < 1900
check be removed anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this PR would cause merge conflicts, even if the solution would be to just obliterate this commit.
While I still don't fully understand the problem, it looks like Bradley's PR #4961 will remove the obsolete |
ITK already dropped Visual C++ 2013 in 2018, with commit 0067eee "COMP: Require MSVC VS14 2015 for C++11 compatibility", by Bradley Lowekamp (@blowekamp).
For the record, Microsoft also dropped VS2013: https://learn.microsoft.com/en-us/lifecycle/products/visual-studio-2013