windows: new -Wreturn-type warnings after #19203 #21355

1. 
   fanquake commented at 3:19 am on March 4, 2021: member

   We now get lots of compile time output like:

   0In file included from test/fuzz/addition_overflow.cpp:7:
   1./test/fuzz/util.h: In member function 'virtual SOCKET FuzzedSock::Get() const':
   2./test/fuzz/util.h:563:5: warning: no return statement in function returning non-void [-Wreturn-type]
   3  563 |     }
   4      |     ^
   5./test/fuzz/util.h: In member function 'virtual SOCKET FuzzedSock::Release()':
   6./test/fuzz/util.h:568:5: warning: no return statement in function returning non-void [-Wreturn-type]
   7  568 |     }
   8      |     ^
   9...
   

   anywhere test/fuzz/util.h is included. Would be great to not have build logs filled with this spam.

   This was all present in the CI. i.e https://cirrus-ci.com/task/5500626558779392?command=ci#L4283.

   cc @practicalswift @vasild

2. fanquake added the label Bug on Mar 4, 2021
3. fanquake added the label Windows on Mar 4, 2021
4. MarcoFalke added the label Tests on Mar 4, 2021
5. MarcoFalke added the label Upstream on Mar 4, 2021
6. 
   MarcoFalke commented at 7:18 am on March 4, 2021: member

   The compiler is broken, you may report this upstream.

   (Edit, unrelated, different compiler bug #21247)

7. 
   vasild commented at 9:22 am on March 4, 2021: member

   Maybe this would silence the compiler to avoid spamming the build logs? Worth doing that even if it is a compiler bug?

    0--- i/src/test/fuzz/util.h
    1+++ w/src/test/fuzz/util.h
    2@@ -554,23 +554,23 @@ public:
    3     }
    4 
    5     ~FuzzedSock() override
    6     {
    7     }
    8 
    9-    SOCKET Get() const override
   10+    [[noreturn]] SOCKET Get() const override
   11     {
   12         assert(false && "Not implemented yet.");
   13     }
   14 
   15-    SOCKET Release() override
   16+    [[noreturn]] SOCKET Release() override
   17     {
   18         assert(false && "Not implemented yet.");
   19     }
   20 
   21-    void Reset() override
   22+    [[noreturn]] void Reset() override
   23     {
   24         assert(false && "Not implemented yet.");
   25     }
   26 
   27     ssize_t Send(const void* data, size_t len, int flags) const override
   28     {
   

8. 
   hebasto commented at 6:51 pm on March 4, 2021: member

   Related to #20586 ?
9. 
   practicalswift commented at 8:26 pm on March 4, 2021: contributor

   Oh, good catch.

   We should probably just return INVALID_SOCKET in those cases (unreachable in practice of course).

10. fanquake closed this on Mar 5, 2021

    ---

11. sidhujag referenced this in commit 7106a1131e on Mar 5, 2021
12. 
    practicalswift commented at 7:16 am on March 6, 2021: contributor

    -Werror=return-type would have caught this pre-merge.

    More generally I think it would make sense to more liberally use of -Werror: among other things it would help us catch warnings that are only present or triggered in a subset of the three major compilers (GCC, Clang and MSVC).

    If anyone wants to add -Werror=return-type (or -Werror:s more generally) I’d be glad to review :)

13. 
    fanquake commented at 7:22 am on March 6, 2021: member

    -Werror=return-type would have caught this pre-merge.

    Looking at CI or build logs would also have caught this pre-merge. We can’t just enable -Werror=return-type because the mingw-w64 compiler emits spurious warnings. In-fact we currently have -Werror disabled for Windows builds in the CI partly for that exact reason. If you want to turn it on, you should start by reviewing #20586.

14. 
    practicalswift commented at 8:11 am on March 6, 2021: contributor

    -Werror=return-type would have caught this pre-merge.

    Looking at CI or build logs would also have caught this pre-merge.

    Oh, the point I was trying to make was that -Werror=return-type would have caught this automatically :)

    Sorry if that was unclear!

15. 
    hebasto commented at 8:18 am on March 6, 2021: member

    @practicalswift automaGically :)
16. DrahtBot locked this on Aug 18, 2022
17. PastaPastaPasta referenced this in commit 7f47a61ff3 on Feb 6, 2023
18. PastaPastaPasta referenced this in commit 0606041022 on Apr 10, 2023
19. PastaPastaPasta referenced this in commit 62b47ddf26 on Apr 15, 2023

Contributors
fanquake MarcoFalke vasild hebasto practicalswift

Labels
Bug Windows Tests Upstream