Entt: [warnings] Bundled warnings (minor) for 3.6.0-pre

[Green-Sky]

I had a little go with the current master (4b89fc390d7bb5cde86143739fffc63ec02310b4) , while having extra warnings enabled.

• unused parameter in core/any.hpp:45 : Type = void, results in parameter to not being used. This looks like intended behavior, so please mark as maybe_unused

• adding a system to a organizer, with nothing but the registry as a parameter results in unused parameters in entity/organizer.hpp:158: parameters buffer and count. This looks intended, please mark as maybe_unused

... i also see "a lot" of sign conversion warnings. But I don't know if they should be silenced.

[skypjack]

From where do you have signed conversion warnings? Or... What warnings did you enable? Maybe it's worth having them also on the CI. :+1:

And thanks for trying it and reporting these!!

[Green-Sky]

From where do you have signed conversion warnings?

mostly inside the string hashing, and mostly implicit signed char to unsigned long conversions, which get caught because of the change of signed-nes ... but some warnings do not make sens (might be my compiler version).

I use -Wall -Wextra -pedantic and in this specific case its gcc 7.5

edit: i do actually have more enabled, see https://github.com/MadeOfJelly/MushMachine/blob/e74154ccee612ab81dd168938e41d46d3e209859/CMakeLists.txt#L31

[skypjack]

mostly inside the string hashing

Ah, wait, do you mean hashed_string? I remember of an issue with VS2017 that issued a wrong warning for a signed conversion.

[Green-Sky]

/home/green/workspace/github/entt/src/entt/core/hashed_string.hpp: In instantiation of ‘static constexpr entt::id_type entt::basic_hashed_string<Char>::helper(const Char*) [with Char = char; entt::id_type = unsigned int]’:
/home/green/workspace/github/entt/src/entt/core/hashed_string.hpp:161:40:   required from ‘constexpr entt::basic_hashed_string<Char>::basic_hashed_string(entt::basic_hashed_string<Char>::const_wrapper) [with Char = char]’
/home/green/workspace/github/entt/src/entt/core/hashed_string.hpp:249:35:   required from here
/home/green/workspace/github/entt/src/entt/core/hashed_string.hpp:78:28: warning: conversion to ‘unsigned int’ from ‘const char’ may change the sign of the result [-Wsign-conversion]
             value = (value ^ static_cast<traits_type::type>(*(curr++))) * traits_type::prime;
                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

this is the full warning. VS? i'm on linux with g++7.5.0 though :sweat_smile:

[skypjack]

As for the other points:

• any: void results in all parameters being unused actually, good catch.
• Indeed, you're right. 👍

[skypjack]

What I don't get of the warning in hashed_string is that there is already an explicit cast that should suppress it: here:

value = (value ^ static_cast<traits_type::type>(*(curr++))) * traits_type::prime;

Where curr is the _string_ (a pointer to the n-th element). It seems to me a wrong warning. Am I missing something?

[Green-Sky]

that there is already an explicit cast that should suppress it

my thoughts exactly !

it might be because of the const that trips up the compiler :man_shrugging:

but this is not the only warning, and the others do make more sense. shall i prepare a pull request, so that we run it on the CI ?

[skypjack]

I made a commit for the other two. Though, yeah, I'll add -Wextra and probably -pedantic to the CI for a better report, even though I personally find the latter a little extreme.

[skypjack]

I've enabled more warnings here. This is what I obtain:

• A couple of warnings from cr, a library used for testing plugins. So, nothing under my direct control nor something you'll have in a production env due to EnTT. We can freely ignore them.
• A warning from a test. It's on purpose actually, even though I see why there is that warning. Also in this case, not in the EnTT headers, so we can freely ignore it.

I've no reports for any conversion instead, not even for GCC 7.5.0.
Am I doing something wrong? I don't really see from where your warning come and I can't reproduce them. :man_shrugging:

[Green-Sky]

First, I see you have Wshadow and pedatic :+1:

Second, I do have more enabled. It looks like you did not click on the link, so I will just paste it here:

    add_compile_options(
        -Wall -Wextra # Reasonable and standard
        -Wpedantic # Warn if non-standard C++ is used
        -Wunused # Warn on anything being unused
        -Wconversion # Warn on type conversions that may lose data
        -Wsign-conversion # Warn on sign conversions
        -Wshadow # Warn if a variable declaration shadows one from a parent context
    )

.... Wunused has already proven useful (first 2 bullet-points of this issue).

[skypjack]

It looks like you did not click on the link

😄 I actually looked into it when you commented and kindly forgot about the link itself during the night. Good point. 👍

[skypjack]

I think I won't address signed conversions. They are a tons and none of these warnings is suspicious.
I don't think that putting static casts all around the code is a good idea too.

I mean, it triggers also if you index an array using an integer rather than an unsigned integer. It's insane.

[Green-Sky]

I don't think that putting static casts all around the code is a good idea too.

yeah i know. Lets keep it reasonable.

[skypjack]

All other warnings are suppressed now and there are more options set on the CI.
I've removed the ones for the conversions, because they are really extreme and somewhat insane.
Any suggestion? At the moment, the CI is pretty clean (only macos has some warnings but they are due to ASSERT_DEATH from gtest).

[Green-Sky]

All other warnings are suppressed now

nice. just had a go with the warning-branch with my engine, and i too, disabled the conversion ones. now i actually see warnings i am responsible for :+1: .

Any suggestion?

nothing much to say here. as you have said, the conversion one are a bit too extreme. i find the other warnings reasonable though. i am happy.

[skypjack]

Nice. I'm closing the issue then. Feel free to reopen it or to create a new one if you find anything else. Thanks.