Adds configuration option WARNING_LEVEL

Activity

CR-26 14

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author & Moderator 48m 5 No it's just ... waiting for better times https://code.re...
    Reviewer - 0% reviewed      
    Reviewer - 0% reviewed 9m 1 What about -Wextra ?
    Reviewer - Complete 19m 1 So this is abandonned ?
    Reviewer - Complete 1h 1m 5 Indeed, the option to lower the warning level for a rando...
    Reviewer - Complete 11m 2 extra spaces or tab between "don't" and "enable".
    Reviewer - 0% reviewed      
    Total   2h 27m 14  
    #permalink

    Objectives

    The patch adds a configuration option WARNING_LEVEL with 3 possible values:
    0 - warnings off, 1 - Normal warnings (some warnings silenced to reduce noise, default), 2 - All warnings on
    Setting 1 will silence a few warnings for all modules and some more for 3rd party modules.
    The patch replaces allow_warnings(module) with set_3rd_party_source(), which will not only allow warnings, but also silence some when WARNING_LEVEL is 1
    instead of passing the target, it modifies the global CMAKE_C_FLAGS etc. The reason is that some of the warning related options do only apply for C or CXX and can thus not be set as COMPILE_FLAGS terget property, which is language unaware.
    macros are used instead of function because functions don't get the scope of the result right for some reason, not even with PARENT_SCOPE. Should probably be fixed for other of these functions as well.
    The patch also adds a function to remove properties / compile flags.

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Thomas Faber

    Excellent! Having more order in this regard should be very useful. I wouldn't...

    Excellent! Having more order in this regard should be very useful.
    I wouldn't like set_3rd_party_source in this form to spread over more code than necessary though (e.g. enabling it for 3rd party stuff that doesn't need it, or even for our own stuff).

    Other nice to have things (in general, not necessarily now) might be:

    • "this is proper code. give me more warnings!" for rtl/ntdll/ntoskrnl and anything else that's important or simply good code
    • special treatment (somewhere in the middle?) for Wine code

    Timo Kreuzer

    We could slightly modify the macro to set_warning_level(NONE), set_warning_le...

    We could slightly modify the macro to
    set_warning_level(NONE), set_warning_level(LOW), set_warning_level(NORMAL) (which wouldn't do anything), set_warning_level(HIGH) and set_warning_level(LOW NOERROR)
    But in fact I think we already enabled all reasonable warnings by default. Or is there anything you would like to add to modules of high quality?
    One fear with "set_Warning_level" is that it quickly gets misused. People will see a warning and think, hey lets just change the warning level and good.
    While for 3rd party code I find it absolutely reasonable to just shut it up (on normal builds, while for syncing/importing it should be normal to be able to catch problems we can fix), I don't like seeing it being used on any of our modules other than setting it to high.
    The question would be how to interct with the global setting. I would suggest having the set_warning_level() macro only take precedence when the global level is 1 (default), when it is 2 all warnings should be on and allow_warning probably globally enabled.
    Comments/thoughts appreciated.

    Thomas Faber

    Indeed, the option to lower the warning level for a random module might be to...

    Indeed, the option to lower the warning level for a random module might be too tempting. I like set_3rd_party_source, since it makes it pretty clear that it's really just for that. As a compromise, something like set_warning_level(3RDPARTY) vs set_warning_level(HIGH) would be an idea (though that's not particularly pretty).
    The interaction with the global setting makes sense as you described it IMO (I was already wondering about the -Werror with level 2).

    As for additional warnings:
    Some I generally find useful in good code are -Wwrite-strings (string literals are const) -Wshadow (shadowed identifiers) -Wstrict-prototypes (function declaration without argument types) -Wredundant-decls (double declaration)
    Some good ones from -Wextra: -Wold-style-declaration (e.g. storage class not first) -Wignored-qualifiers (e.g. function returns const int) -Wtype-limits (comparison is always true/false due to limited range of data type)

    Sylvain Petreolle

    So this is abandonned ?

    So this is abandonned ?

    Timo Kreuzer

    No it's just ... waiting for better times https://code.reactos.org/static/olp...

    No it's just ... waiting for better times

    /dll/opengl/glu32/CMakeLists.txt Changed
    Open in IDE #permalink
    /lib/3rdparty/libxml2/CMakeLists.txt Changed
    Open in IDE #permalink
    /base/applications/atactl/CMakeLists.txt Changed  
    Open in IDE #permalink
    /base/applications/.../wlanconf/CMakeLists.txt Changed  
    Open in IDE #permalink
    /base/applications/winhlp32/CMakeLists.txt Changed  
    Open in IDE #permalink
    /base/applications/write/CMakeLists.txt Changed  
    Open in IDE #permalink
    /base/shell/explorer/CMakeLists.txt Changed  
    Open in IDE #permalink
    /cmake/compilerflags.cmake Changed  
    Open in IDE #permalink
    /cmake/config.cmake Changed   1
    Open in IDE #permalink
    /cmake/gcc.cmake Changed   2
    Open in IDE #permalink
    /cmake/msvc.cmake Changed   2
    Open in IDE #permalink
    /dll/3rdparty/libtiff/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/3rdparty/libxslt/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/directx/ksproxy/CMakeLists.txt Changed   3
    Open in IDE #permalink
    /dll/directx/quartz/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/directx/wine/ddraw/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/directx/wine/wined3d/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/glu32/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/.../auxiliary/CMakeLists.txt Changed   1
    Open in IDE #permalink
    /dll/opengl/.../wgl/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/.../libgl-gdi/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/mesa/src/glsl/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/mesa/.../mapi/glapi/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/opengl/mesa/src/mesa/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/aclui/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/advpack/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/browseui/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/crypt32/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/dbghelp/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/itss/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/localspl/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/mciseq/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/mciwave/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/msxml3/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/rpcrt4/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/shdocvw/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/shell32/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/spoolss/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/windowscodecs/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/winmm/CMakeLists.txt Changed  
    Open in IDE #permalink
    /dll/win32/xmllite/CMakeLists.txt Changed  
    Open in IDE #permalink
    /drivers/bluetooth/fbtusb/CMakeLists.txt Changed  
    Open in IDE #permalink
    /drivers/filesystems/ext2/CMakeLists.txt Changed  
    Open in IDE #permalink
    /drivers/storage/ide/uniata/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/adns/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/cardlib/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/freetype/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/fullfat/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/libmpg123/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/3rdparty/libxml2/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/drivers/lwip/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/fslib/ext2lib/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/pseh/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/rossym_new/CMakeLists.txt Changed  
    Open in IDE #permalink
    /lib/sdk/crt/msvcrtex.cmake Changed  
    Open in IDE #permalink
    /win32ss/drivers/.../vga_new/CMakeLists.txt Changed  
    Open in IDE #permalink

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against