Incorrect description of `-Wenum-constexpr-conversion`

[pogo59]

In https://discourse.llvm.org/t/clang-16-notice-of-potentially-breaking-changes/65562 @AaronBallman describes -Wenum-constexpr-conversion this way:

• Clang will now correctly diagnose as ill-formed a constant expression where an enum without a fixed underlying type is set to a value outside the range of the enumeration’s values.

    enum E { Zero, One, Two, Three, Four };
    constexpr E Val1 = (E)3;  // Ok
    constexpr E Val2 = (E)7;  // Ok
    constexpr E Val3 = (E)8;  // Now diagnosed as out of the range [0, 7]
    constexpr E Val4 = (E)-1; // Now diagnosed as out of the range [0, 7]

I think the actual "range" of the enumeration is [0, 4] not [0, 7] and therefore clang should diagnose the second example (E)7.

Alternatively, the diagnostic should be described differently, in order to accurately specify the conditions that are being diagnosed. Perhaps instead of "outside the range of the enumeration's values" it should be "outside the range of the minimum-size bitfield needed to represent the range of the enumeration's values."

But I'd rather see this handled as a bug, and fixed accordingly.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[AaronBallman]

The range of values that can be represented by that unscoped enumeration is [0, 7], not [0, 4], so I'm not certain I see where the bug is here. See http://eel.is/c++draft/enum#dcl.enum-7 for standards details on it.

The current behavior is certainly correct in terms of constant expression evaluation. However, because we've never diagnosed that situation as invalid for constexpr before and because we knew it would break code, we added a warning category so that users have some time to adjust their code bases before it becomes a hard error.

It sounds like what you want is a different diagnostic when forming an enumeration type from a value that's not contained in the enumeration. -Wassign-enum comes close to that in that it's looking at the range of values present in the enumeration, but it misses "gaps" in the enum. e.g., enum E { Zero, One, Three = 3, Four }; E e = 2; will not be diagnosed but E e = 5; will be.

[pogo59]

The paragraph you cite does not define "range of values" it defines "underlying type." As a programmer, these are different things; the enumeration's range of values is clearly [0, 4] while the underlying type's range of values is, for this particular implementation, [0, 7]. (The standard doesn't say it has to be the minimum size integral type, AFAICT; just that it be some integral type.) I would want the diagnostic to tell me about the underlying type's range of values, not the enumeration's range of values. They are different.

Incidentally I'm unable to find the string "enum_constexpr_conversion" anywhere in the clang tree, should I be looking for something else?

[pogo59]

The standard doesn't say it has to be the minimum size integral type, AFAICT; just that it be some integral type.

FTR, having that type be smaller than char (as in this example) is surprising to me.

[shafik]

I definitely see how the diagnostic can feel weird but if we look at [decl.enum]p8 it says:

Otherwise, the values of the enumeration are the values representable by a hypothetical integer type with minimal width M such that all enumerators can be represented.

and so that is why we adopted that language.

Also note UBSan -fsanitize=enum also uses this convention for range of values for an enum see the following godbolt: https://godbolt.org/z/9vcMxKe3P

enum E {};

int main() {
    E e = static_cast<E>(2);
    return e;
}

and this is also consistent with -fstrict-enums which we can see the result here: https://godbolt.org/z/qcsc7n9sT and in particular look for the load:

%0 = load i32, ptr %e, align 4, !dbg !26, !tbaa !22, !range !27

which has a !range !27 and we can see !27 denotes the range of values as [0,2):

!27 = !{i32 0, i32 2}

Note !range is documented here: https://llvm.org/docs/LangRef.html#range-metadata

[pogo59]

UBsan says "not a valid value" without saying anything about ranges, and it's clearly not a valid value, so I have no problem with that message.
I can't see any diagnostics in the second godbolt example.

I'd prefer the message to be easily understood by people (like me) who are not experts in such fine details of the standard. I'm befuddled by the decision to encompass a small number of additional values not defined explicitly in the enumeration, but then a lot of things about C++ are befuddling.

It would probably be sufficient to omit the [0, 7] from the diagnostic, as that's the part causing all the confusion.

[AaronBallman]

It would probably be sufficient to omit the [0, 7] from the diagnostic, as that's the part causing all the confusion.

I would be pretty strongly opposed to removing that information because that's the most relevant piece of info in the diagnostic (aside from getting a diagnostic at all). It's telling you "values inside this range are fine, values outside this range will get you the diagnostic", so removing that information makes the diagnostic too mysterious.

I can sympathize with the ambiguity of using "range" in the wording, but I think that really is the correct phrase to use here. There is a range of valid values for an enumeration without a fixed underlying type, and that range is determined by the smallest bit-field which can hold all of the values. Your suggestion to reword the diagnostic to talk about that smallest bit-field would make the diagnostic even less beginner friendly (not many folks know about what bit-fields are or how they differ).

Also, use of the phrase "range" for this is not unique to Clang:

https://wiki.sei.cmu.edu/confluence/display/cplusplus/INT50-CPP.+Do+not+cast+to+an+out-of-range+enumeration+value
https://en.cppreference.com/w/cpp/language/enum
https://pvs-studio.com/en/docs/warnings/v1016/

[pogo59]

So really it's the language's fault for defining a confusing range for enums.

BTW I see that cppreference in fact says it "is in range if it would fit in the smallest bit field large enough to hold all enumerators" so what I think of as a common reference does use that exact language. I think it's not as out-in-left-field as you do.

I think my users are far more likely to understand what a bitfield is than why a diagnostic is telling them about a range that makes no intuitive sense.

[pogo59]

But, if you're really adamant about not changing the wording, there's no reason to leave this open. I've said my piece, and downstream documentation to explain it should keep the service calls to a minimum.

[AaronBallman]

But, if you're really adamant about not changing the wording, there's no reason to leave this open. I've said my piece, and downstream documentation to explain it should keep the service calls to a minimum.

It's not so much adamant about not changing the wording as not knowing what words will actually be an improvement. We did a reasonable amount of bikeshedding on the diagnostic wording to get it to this point, so further bikeshedding should lead to clear improvements over the status quo. I'm not certain there are clear improvements to be made here because the current wording is correct and presents all of the information required to fix the issue. I think adding "smallest bit-field large enough" wording will make the diagnostic harder to understand because that particular bit of information isn't actually all that relevant except to explain how the language standard works. (Also, that wording would be a little bit imprecise for a case like enum E {} because the smallest bit-field large enough to hold "all enumerators" is a zero-length bit-field, but that's not how the language works either.)

So really it's the language's fault for defining a confusing range for enums.

Yes; this particular rule is one where C++ diverged from C, and it really doesn't seem to carry its weight in terms of utility IMO.