On Mon, Oct 16, 2017 at 01:06:04AM +0100, Jonathan Wakely wrote:
> On 14 October 2017 at 17:19, Sam van Kampen via gcc <[email protected]> wrote:
> > On Sat, Oct 14, 2017 at 04:43:33PM +0100, Jonathan Wakely wrote:
> >> On 14 October 2017 at 15:50, Sam van Kampen wrote:
> >> > Dear maintainers,
> >> >
> >> > Having come across https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414
> >> > (bug #61414) quite often myself I decided I wanted to fix it.
> >> >
> >> > By reading through parts of the GCC internals manual I have
> >> > managed to add a warning flag, the code for which I will submit to
> >> > gcc-patches, but since this is my first time contributing to GCC I have
> >> > some questions.
> >> >
> >> > 1. When it comes to the patch, should I post it to the bug tracker
> >> > before sending it to gcc-patches?
> >>
> >> No. Patches go to the mailing list, not bugzilla.
> >>
> >> But you can add the URL of the gcc-patches mail to bugzilla (and then
> >> somebody should add the "patch" keyword to the bug).
> >
> > Noted.
> >
> >> > 2. A better fix for this bug would be to only warn when the number
> >> > of defined enumerators would not fit inside a bitfield of the
> >> > specified size. Seeing as I don't know the GCC internals very
> >> > well, I'm wondering if anyone could point me in the right
> >> > direction when it comes to getting the number of defined
> >> > enumerators in an enum type.
> >>
> >> Why does the number of enumerators make any difference?
> >
> > I suppose the number of enumerators does not make any difference, but
> > rather the enumerator with the highest integer value. For instance, in
>
> And the lowest value, consider enum X { Y = -200, Z = 5 };
>
> > this example:
> >
> > enum class A { A, B, C; };
> > struct D { A enum_field : 2; };
> >
> > the enumerator with the highest integer value is C, with an integer
> > value of 2. This enum type therefore fits inside the bit-field in
>
> But it doesn't, because as I think it says in the bug report, a scoped
> enum (i.e. enum class) has a fixed underlying type, and all values of
> the underlying type are valid values of the enumeration type.
>
> enum class A { a, b, c };
> A max = A{std::numeric_limits<std::underlying_type_t<A>>::max()};
>
Fair enough, I suppose that is the way they are defined in the standard
(and that is the document we have to follow, after all). I wish there
was a way of defining scoped enums that have a precision that is lower
than eight bits, but it is what it is.
> > struct D, and so the compiler should not emit a warning in my eyes.
> >
> > Of course, you can cast an integer to an enum type and assign it that
> > way, which should probably trigger a warning if the value could be too
> > large (it already triggers -Woverflow if the value is a compile-time
> > constant).
>
> As I said in the bug report, PR 51242 comment 27 suggests a more useful
> warning.
>
>
Right. On the code:
enum class FooEnum {A,B,C,D,E,F};
struct Bar { FooEnum baz:2; };
Bar b; b.baz = FooEnum::F;
clang 5.0.0 warns with:
warning: implicit truncation from 'FooEnum' to bit-field changes value
from 5 to 1 [-Wbitfield-constant-conversion]
(when adding -Wbitfield-enum-conversion, clang warns with an error that
is similar to the error gcc gives by default, but points at an
assignment (whether it fits or not) instead of a declaration)
and gcc (trunk) warns with:
warning: overflow in conversion from ‘FooEnum’ to ‘signed char:2’
changes value from ‘(FooEnum)5’ to ‘1’ [-Woverflow]
Neither compiler warns when a value is assigned that is not known to
fit. So what do you propose:
1. Keeping the current warning on declaration and warn if we don't
know whether a value fits?
2. Keeping the current warning and warning when we know a value
does not fit?
3/4. Dropping the current warning and doing one of the above?
In any case, I'll send the flag-only patch to gcc-patches, since that
and a discussion on other warnings to add are logically separate changes.
> >> > 3. When it comes to documentation and tests, I've documented the
> >> > flag in doc/invoke.texi and will add a test in the test suite,
> >> > hopefully when the patch includes the check specified in 2. Are
> >> > there are any other places I should add documentation?
> >>
> >> No, that's all.
> >
> > Thanks.
> >
> > Sam
