Improve all warnings

[ffissore]

See 61592d7#commitcomment-10668365

[matthijskooijman]

In that comment, @cmaglie wrote:

Sorry I was a bit in a hurry, here a less terse comment ;-)

IMHO it should be implemented in a way similar to what we do for tools verbosity:

tools.avrdude.bootloader.params.verbose=-v
tools.avrdude.bootloader.params.quiet=-q -q
tools.avrdude.bootloader.pattern="{cmd.path}" "-C{config.path}" {bootloader.verbose} -p{build.mcu} -c{protocol} {program.extra_params} "-Uflash:w:{runtime.platform.path}/bootloaders/{bootloader.file}:i" -Ulock:w:{bootloader.lock_bits}:m

in this case tools.avrdude.bootloader.params is loaded with the contents of tools.avrdude.bootloader.params.quite if the verbosity is off or with the contents of tools.avrdude.bootloader.params.verbose if the verbosity is on.

For the "warning level" setting we can do something like:

compiler.warning_flags.none=-w
compiler.warning_flags.normal=
compiler.warning_flags.all=-Wall
compiler.warning_flags.extra=-Wall -Wextra

recipe.c.o.pattern="{compiler.path}{compiler.c.cmd}" {compiler.c.flags} {compiler.warning_flags} ............

It seems this was implemented in b42c666 already. I haven't looked closely at that commit yet, but I was writing a response to @cmaglie's proposal already when I found that commit.

I totally agree with @cmaglie's comment - hardcoding these flags is not the way to go. It's a bit of a pity that there is no PR for this change to discuss things before merging into master, now the master branch is seeing multiple different versions of this feature.

The approach proposed seems good to me. I even like it better than the bootloader params one, since the {compiler.warning_flags} replacement refers to e.g. compiler.warning_flags.none more clearly than {bootloader.verbose} to e.g. tools.avrdude.bootloader.params.verbose.

Ideally, the warning levels aren't fixed, but the list of possible values are taken from platform.txt, and some platforms can define more, less, or different values. This does imply that the preferences might need to be stored per-platform, which can become confusing quickly. So perhaps a fixed list of warning levels might be batter after all.

However, the current list of levels doesn't work for me. In particular, "all" vs "extra" is confusing - "all" is normally understood to mean all warnings possible, it's weird that "extra" is more than "all". This is weird in gcc, but I don't think this weirdness should spill over into the IDE. Something like "none", "default", "more" and "most" (or "all") might be better?

Before, it was also discussed to have different warning levels for the sketch, the core and libraries. The rationale for this was that mosts users won't have any idea how to fix warnings in the core, and also in libraries they are using, so there is no need in distracting them with those warnings. However, for library authors and core hackers, having an easy way to enable these warnings is essential to improve the quality of the code. One can argue that suppressing warnings in the core for normal users isn't necessary if we make sure they never appear, but in practice some might sneak through, and new compiler versions introduce new warnings, so hiding those from novice users seems like a good idea.

Another idea was to actively filter the list of warnings, using various -W flags. A lot of warnings are useful for novice users, but some are only confusing. By enabling warnings individually, this could lead to a "novice-proof" selection of warnings. Not sure how to call this level, maybe "selected" or something like that?

[ffissore]

You're right @matthijskooijman. First warning fix was just a boolean flag and I didn't thought it was worth a PR. However @cmaglie suggestion was far more complex and deserved a PR, which I failed to make. I'll keep on implementing your suggestion (better names) in a PR asap.
I agree having too precise warning level selection may be confusing for users and hard for us to make "nice and easy to use": I prefer to stick with some predefined warning levels. They are used by advanced users only anyway: they can easily filter warnings belonging to their libs/cores

[matthijskooijman]

They are used by advanced users only anyway: they can easily filter warnings belonging to their libs/cores

I really disagree here - I think that a lot of warnings are useful to novice users too, but only the warnings concerning their sketch code. Warnings in library and core code is mostly confusing to them.

[ffissore]

Is there a gcc flag that tells gcc to log warnings only when compiling a certain file, or should sketch-only-warnings be achieved by turning warnings on depending on the file being compiled?

[matthijskooijman]

Every file is compiled seperately, so you should just modify the compiler flags for every gcc invocation. Gcc doesn't actually know about "core" vs "library" vs "sketch", so I don't think there is any other way to approach this. Or am I misunderstanding your question?

[ffissore]

That's what I suspected. It's unfortunate because it means we had to fiddle with gcc command line in a similar way I was doing with the first warning boolean flag (and that I changed after @cmaglie suggestion). As an alternative, we had to add a sketch specific recipe in platform.txt

[matthijskooijman]

That doesn't seem to be the case - you just apply a different warning level to sketch, library and core, calculating the recipe separately for each. There is no need to hardcode any values that actually appear on the commandline.

This means that compiling, say, sketch and libraries with level "all" results in the exact same commandline for both, so platform.txt cannot specify different warning levels or different options for a single warning level based on if you're compiling a sketch, library or core. But I don't think there is any need to?

[ffissore]

I see your point. We could have two dropdowns, one for the sketch, one for the rest. The related {compiler.sketch.warning_flags} and {compiler.warning_flags} variables could be injected as {compiler.warning_flags} thus avoiding a specific recipe for the sketch. Is that how you pictured it?

[matthijskooijman]

Yes, exactly (though I envisioned a compiler.sketch.warning_flags, compiler.libraries.warning_flags, compiler.core.warning_flags, but perhaps just sketch and "the rest" is also a sufficient distinction indeed.