Fix MinGW build #2298
Fix MinGW build #2298
Conversation
|
cc @tamird |
|
I didn't test msvc, but the behavior is unchanged there, since |
|
MSVC should be tested in appveyor.
…On May 15, 2017 12:11 PM, "Orgad Shaneh" ***@***.***> wrote:
I didn't test msvc, but the behavior is unchanged there, since _MSC_VER
is defined.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2298 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPHBQXDQtXMMM0QBxy2IhaIplqjK_ks5r6HkfgaJpZM4NbQcK>
.
|
|
@tamird Sadly it looks like the AppVeyor build is already broken :-( https://ci.appveyor.com/project/Facebook/rocksdb/branch/master/messages |
|
Heh, indeed. I guess this PR should wait until that's fixed. Disappointing that #2269 was merged with failing CI. |
|
@orgads updated the pull request - view changes |
port/win/port_win.h
Outdated
| // VS 15 | ||
| #if (defined _MSC_VER) && (_MSC_VER >= 1900) | ||
| // VS 15 / MinGW | ||
| #if !defined(_MSC_VER) || (_MSC_VER >= 1900) |
There was a problem hiding this comment.
that does not sound right to me, why will we enable this if _MSC_VER is not defined
There was a problem hiding this comment.
Because I assume that MinGW/GCC supports C++11. Why not?
There was a problem hiding this comment.
port_win.h shouldn't enable for non-Windows. Looks like we should use __MINGW32__ macro to tell if it is MinGW?
There was a problem hiding this comment.
It's equivalent; this checks for MSVC. Is this change really worth more CI cycles?
There was a problem hiding this comment.
If you wanted to be pedantic about it, you could write this as:
#if (defined _MSC_VER) && (_MSC_VER < 1900)
<old MSVC code>
#else
<normal code>
#endif
There was a problem hiding this comment.
Moreover, I don't think there's a reason to exclude clang or ICC for example. Do they define __MINGW32__?
There was a problem hiding this comment.
ok, I misread the code. I thought without a mingw-specific macro the code will compile for non-windows.
btw. does MSVC has std::snprintf in <cstdio>? If so we can probably update all occurrence of snprintf to std::snprintf and get rid of the macro?
There was a problem hiding this comment.
According to the docs it was only added in MSVC2015.
There was a problem hiding this comment.
@orgads looks like CI hasn't started on this yet (and won't for a while). Perhaps it's worth rewriting it in the style I suggested above, for clarity?
|
@yiwu-arbug It looks like AppVeyor isn't enabled for PRs like Travis is. Could we enable it somehow? |
|
@adamretter looks like it's still building them, but the status isn't being shown in GH. |
|
@tamird Interesting. I think it would be good if we could get the AppVeyor "check" to show up on the PRs as well. Looks like this PR passed though, so I am happy with it. |
|
@adamretter: @sdwilsh help fix an auth issue and the AppVeyor status should be back next time. |
port/win/port_win.h
Outdated
| // VS 15 | ||
| #if (defined _MSC_VER) && (_MSC_VER >= 1900) | ||
| // VS 15 / MinGW | ||
| #if !defined(_MSC_VER) || (_MSC_VER >= 1900) |
There was a problem hiding this comment.
port_win.h shouldn't enable for non-Windows. Looks like we should use __MINGW32__ macro to tell if it is MinGW?
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@orgads updated the pull request - view changes - changes since last import |
|
@orgads updated the pull request - view changes - changes since last import |
|
@orgads updated the pull request - view changes - changes since last import |
|
@orgads updated the pull request - view changes - changes since last import |
|
@orgads updated the pull request - view changes - changes since last import |
|
@orgads updated the pull request - view changes - changes since last import |
|
Rebased. |
|
@orgads updated the pull request - view changes - changes since last import |
|
Rebased again. Hello? |
|
@orgads updated the pull request - view changes - changes since last import |
snprintf is defined as _snprintf, which doesn't exist in the std namespace. blob_file.cc: In member function 'std::__cxx11::string rocksdb::blob_db::BlobFile::DumpState() const': blob_file.cc:92:3: error: '_snprintf' is not a member of 'std' std::snprintf(str, sizeof(str), ^~~
|
@orgads has updated the pull request. |
|
@yiwu-arbug Rebased and fixed debug and release builds. Please review. |
|
@orgads any idea why AppVeyor is failing? |
|
@orgads has updated the pull request. |
|
@yiwu-arbug commented on Sep 14, 2017, 7:25 PM GMT+3:
Probably because of the second commit. I removed it for now, let's see if it passes and then I'll try to fix release build in a separate commit. |
|
Looks good now. |
|
@yiwu-arbug: Well? |
|
Sorry for delay. Merging. |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
snprintf is defined as _snprintf, which doesn't exist in the std
namespace.