ci: go back to stricter checks#1012
Conversation
Current Aviator status
This PR was merged using Aviator. See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
|
What's there for me to review? Should we fix first? |
|
I think it'd make sense to merge then rebase all PRs that work towards getting the build green again? |
4fa994a to
3a734ed
Compare
3a734ed to
dfc7422
Compare
f7f8dcb to
970ed1e
Compare
|
This pull request failed to merge: some CI status(es) failed. Remove the Failed CI(s): Check ubuntu-20.04 (devel), Check ubuntu-20.04 (oldrel-2) |
82fc1b3 to
0dfb8d1
Compare
4d23cb7 to
e3e43ce
Compare
|
This pull request failed to merge: some CI status(es) failed. Remove the Failed CI(s): Check ubuntu-20.04 (oldrel-2) |
|
This pull request failed to merge: some CI status(es) failed. Remove the Failed CI(s): Check windows-latest (release), Check ubuntu-20.04 (oldrel-3), Check ubuntu-20.04 (release), Check macOS-latest (release), Check ubuntu-20.04 (oldrel-1), Check ubuntu-20.04 (devel), Check ubuntu-20.04 (oldrel-2) |
krlmlr
left a comment
There was a problem hiding this comment.
Thanks! Should be good now.
|
This pull request failed to merge: some CI status(es) failed. Remove the Failed CI(s): Check macOS-latest (release) |
|
@Antonov548: We now have compilation warnings on a few platforms only, e.g., on macOS: Is this dead code? We can remove in There are more failures on Windows. Can you please push to this branch? |
e4ee229 to
323e63a
Compare
Would we like to keep synchronized generated code with |
|
We reconcile at some point, yes, but for now, drift is fine. I want to move
to autogenerating everything, any Stimulus changes might have to be
reverted anyway.
We can edit interface.c and aaa-auto.R directly until after the release.
Michael Antonov ***@***.***> schrieb am Fr. 12. Jan. 2024 um
16:06:
… @Antonov548 <https://github.com/Antonov548>: We now have compilation
warnings on a few platforms only, e.g., on macOS:
rinterface.c:6349:84: warning: variable 'c_outfile' is uninitialized when used here [-Wuninitialized]
rinterface.c:8381:58: warning: variable 'c_instream' is uninitialized when used here [-Wuninitialized]
rinterface.c:8441:59: warning: variable 'c_outstream' is uninitialized when used here [-Wuninitialized]
rinterface.c:10886:33: warning: variable 'c_a' is uninitialized when used here [-Wuninitialized]
rinterface.c:10886:38: warning: variable 'c_b' is uninitialized when used here [-Wuninitialized]
rinterface.c:10886:43: warning: variable 'c_eps' is uninitialized when used here [-Wuninitialized]
rinterface.c:10910:31: warning: variable 'c_a' is uninitialized when used here [-Wuninitialized]
rinterface.c:10910:36: warning: variable 'c_b' is uninitialized when used here [-Wuninitialized]
rinterface.c:10910:41: warning: variable 'c_eps' is uninitialized when used here [-Wuninitialized]
rinterface.c:10917:10: warning: variable 'r_result' is uninitialized when used here [-Wuninitialized]
rinterface.c:11610:10: warning: variable 'r_result' is uninitialized when used here [-Wuninitialized]
rinterface.c:11632:10: warning: variable 'r_result' is uninitialized when used here [-Wuninitialized]
rinterface.c:11646:28: warning: variable 'c_igraph_errno' is uninitialized when used here [-Wuninitialized]
rinterface.c:11653:10: warning: variable 'r_result' is uninitialized when used here [-Wuninitialized]
rinterface.c:11761:31: warning: variable 'version_string' is uninitialized when used here [-Wuninitialized]
rinterface.c:11762:31: warning: variable 'major' is uninitialized when used here [-Wuninitialized]
rinterface.c:11763:31: warning: variable 'minor' is uninitialized when used here [-Wuninitialized]
rinterface.c:11764:31: warning: variable 'subminor' is uninitialized when used here [-Wuninitialized]
Is this dead code? We can remove in rinterface.c and reconcile later, we
have drift anyway.
There are more failures on Windows.
Can you please push to this branch?
Would we like to keep synchronized generated code with functions.yml?
—
Reply to this email directly, view it on GitHub
<#1012 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANJGSZPRLZECFYX2IUTSB3YOFGQJAVCNFSM6AAAAABAZLGH5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZGQ2TQNBXHE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Beware that most of these "uninitialized when used here" warning indicate actual bugs! If the affected function is used, that's a problem we need to fix now. Hopefully none of them are used? |
|
Now there's only Windows and macOS failing, for good reason, I think. |
Yes, checking |
| igraph_errorf("Expecting a scalar integer but received a vector of length %zu.", | ||
| __FILE__, __LINE__, IGRAPH_EINVAL, (size_t) Rf_xlength(value)); | ||
| igraph_errorf("Expecting a scalar integer but received a vector of length %d.", | ||
| __FILE__, __LINE__, IGRAPH_EINVAL, Rf_xlength(value)); |
There was a problem hiding this comment.
@Antonov548 Why did you make this change? %d is only for int, and the return type of Rf_xlength() is system-dependent (may not be compatible with int!), but always fits in size_t. I chose %zu and a cast to size_t on purpose.
There was a problem hiding this comment.
zu is not compatible with some versions of compiler. Maybe good solution will be to use lu?
There was a problem hiding this comment.
What specific compiler is it not compatible with?
There was a problem hiding this comment.
zu is standard in both C99 and C++11. Could it be that the proper standard is not set? I'd be very surprised to see any still-relevant compiler not support it.
There was a problem hiding this comment.
According to tests wit Godbolt, works with Clang 3.4 and GCC 4.8. igraph itself wouldn't support anything earlier.
There was a problem hiding this comment.
@szhorvat https://stackoverflow.com/questions/68900199/how-to-get-mingw-gcc-to-recognize-the-zu-format-specifier-for-size-t
Hm, looks like there still no agreement for zu.
There was a problem hiding this comment.
What that SO answer is saying is that it does actually work but MinGW's GCC incorrectly warns about it. That is very disappointing.
You should be aware that on Windows the long type is 32-bit wide. Using long here is actually wrong. Can't we just use -Wno-format specifically on Windows?
There was a problem hiding this comment.
@Antonov548 Can you try to use PRIuPTR instead? Here is an example:
https://godbolt.org/z/jcWrvWo9b
PRIuPTR should be defined to a string that is the appropriate format specifier for uintptr_t (which should fit size_t). I'm curious to see what this is with MinGW.
There was a problem hiding this comment.
Seems uintptr_t works well. Let's see in ci.
I guess it's better to leave all warning enabled.
There was a problem hiding this comment.
Yes, it's better to leave warnings enabled on all platforms.
Someone just told me about the __USE_MINGW_ANSI_STDIO macro. Defining this to either 1 or 0 may help (unclear).
But if PRIuPTR works, let's just stick to that, as it is standard.
|
@Antonov548 thanks! What are the windows and sanitizer failures? |
|
I see #1110 |
On windows there is warning in |
|
True, |
|
@Antonov548 As a general rule, never use |
9e8249a to
b98dc71
Compare
Fix #1006
Now I understand why everything was green on CI!