Enable more Warnings!

[Domkeykong]

-Wall -Wextra are the bare minumun of warnings to enable
-Wmissing-declarations -Wformat=2 are a good step in the right direction, but
gcc is known to have bad default warning options.
Here is an incomprehansive list of warnings to consider enabeling:

-Wshadow # warn the user if a variable declaration shadows one from a parent context
-Wnon-virtual-dtor # warn the user if a class with virtual functions has a non-virtual destructor.
                   # This helps catch hard to track down memory errors
-Wold-style-cast # warn for c-style casts
-Wcast-align # warn for potential performance problem casts
-Wunused # warn on anything being unused
-Woverloaded-virtual # warn if you overload (not override) a virtual function
-Wpedantic # warn if non-standard C++ is used
-Wconversion # warn on type conversions that may lose data
-Wsign-conversion # warn on sign conversions
-Wnull-dereference # warn if a null dereference is detected
-Wdouble-promotion # warn if float is implicit promoted to double
-Wformat=2 # warn on security issues around functions that format output (ie printf)
-Wmisleading-indentation # warn if indentation implies blocks where blocks do not exist
-Wduplicated-cond # warn if if / else chain has duplicated conditions
-Wduplicated-branches # warn if if / else branches have duplicated code
-Wlogical-op # warn about logical operations being used where bitwise were probably wanted
-Wuseless-cast # warn if you perform a cast to the same type

[Hendiadyoin1]

I just tested with all the flags you specified and in the Kernel accessed code there are like 220 warning, most of them are old-style casts (129) and unsafe casts of integers of diffrerent sizes and signs(53)...
And a zero-sized array in StringImpl?
And zero-sized arrays in Array and Vector

[awesomekling]

Yes, compilers have many warnings. Do you have a specific plan to enable one of these warnings and deal with the fallout?

[Hendiadyoin1]

I'll look into the oldstyle casts a bit and try to fix that 0 sized array in StringImpl

[Nicholas-Baron]

I have started looking for the free warnings (ones which can be enabled without any new warnings occuring).

A few things to report:

1. -Wdouble-precision occurs a lot. Any call to String::format seems to cause double promotion.
2. -Wduplicated-branches hit its first snag in LibWeb/Layout/BlockFormattingContext.cpp around line 374. Deduplicating the branch is tricky, as the spec language comments imply that there is unfinished work.

I will look into the warnings for virtuals (-Wnon-virtual-dtor and -Woverloaded-virtual) next.

[Nicholas-Baron]

-Wnon-virtual-dtor was solved by sprinking in some protected default destructors.
The first warning for -Woverloaded-virtual seems to be intentional (i.e. the code is probably exploiting the overloading).

I am now attempting to work on -Wdouble-promotion, which mostly involves adding double { x } where the cast is needed or changing some doubles to floats.

[Nicholas-Baron]

Status Report as of 15-Apr-2021

Options so far implemented:

-Wcast-align
-Wduplicated-cond
-Wformat=2
-Wlogical-op
-Wmisleading-indentation
-Wnon-virtual-dtor
-Wunused 

In the works:

-Wdouble-promotion #6355
-Wold-style-cast #5524

Still to do:

• -Wshadow
• -Woverloaded-virtual: in some places, this looks intentional and may require larger reworking.
• -Wpedantic
• -Wconversion
• -Wsign-conversion
• -Wnull-dereference
• -Wduplicated-branches: see here for one example.
• -Wuseless-cast

[Nicholas-Baron]

-Wnull-dereference is really yelling about userland code. I think getting just the kernel using it would be a good start, as null derefs in the kernel are always a "whole system crash".

[Nicholas-Baron]

The biggest issue for -Wnull-dereference so far has been AK::WeakPtr. A brief glance at the class suggests to me that a confluence of forces makes this hard.

1. AK::WeakPtr, in user land, has pointer operators defined.
2. This leads to programmers using it as a regular pointer.
3. All the defined pointer operators possibly return nullptr,
4. This normally is no problem for most code, but
5. g++ does not care and assumes that if it can return nullptr, it will return nullptr.

This is too much for now and will probably need a redesign of the class.

[Nicholas-Baron]

-Wshadow does not like int stat(const char*, struct stat*) and struct stat {} having the same name (i.e. int stat() shadows the constructor of struct stat). It also does not seem to care about lambda captures and just assumes that it captures everything, equivalent to [&].

[Nicholas-Baron]

-Wuseless-cast does not like the casts in the AHCI code.
However, g++ cannot bind a packed field, which the code does a lot.
I will try to apply the warning only to userland code and see if that is possible.

[Nicholas-Baron]

-Wuseless-cast does not like the generated code from the WrapperGenerator.
However, it seems to be an easier change in userland (removing const_cast and old-style casts).

[Nicholas-Baron]

-Wconversion does not like converting from enums to bit-fields. Since this commonly occurs in the kernel, userland is probably a better start for it.