[clang][diagnostics] Update note_constexpr_invalid_cast to use enum_select and adjust its uses #130868
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Ayokunle Amodu (ayokunle321) ChangesHandles #123121 @erichkeane Hi, I’m working on converting The patch that introduced By "magic numbers," I understand the patch refers to hardcoded values like /* ThingIWantSelected */ 5, which lack clarity. Could you provide some concrete examples from the source code where this applies? Also, I’ve yet to come across any enum specifically made for a select. Could you clarify when that is necessary and give an example? The note about "Ones that are 'calculated' based on some other criteria aren't a great" isn’t entirely clear to me. Could you elaborate on this? Lastly, I’d appreciate it if you could review the statements I’ve converted so far to ensure I’m on the right track. Full diff: https://github.com/llvm/llvm-project/pull/130868.diff 1 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index ac53778339a20..aae6ce0850ae2 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -131,17 +131,21 @@ def note_constexpr_past_end : Note<
"dereferenced pointer past the end of %select{|subobject of }0"
"%select{temporary|%2}1 is not a constant expression">;
def note_constexpr_past_end_subobject : Note<
- "cannot %select{access base class of|access derived class of|access field of|"
- "access array element of|ERROR|"
- "access real component of|access imaginary component of}0 "
- "pointer past the end of object">;
+ "cannot %enum_select<AccessType>{"
+ "%BaseClassAccess{access base class of}|%DerivedClassAccess{access derived class of}|"
+ "%FieldAccess{access field of}|%ArrayElementAccess{access array element of}|"
+ "%ErrorAccess{ERROR}|"
+ "%RealComponentAccess{access real component of}|%ImaginaryComponentAccess{access imaginary component of}"
+ "}0 pointer past the end of object">;
def note_non_null_attribute_failed : Note<
"null passed to a callee that requires a non-null argument">;
def note_constexpr_null_subobject : Note<
- "cannot %select{access base class of|access derived class of|access field of|"
- "access array element of|perform pointer arithmetic on|"
- "access real component of|"
- "access imaginary component of}0 null pointer">;
+ "cannot %enum_select<NullSubobjectAccess>{"
+ "%BaseClassAccess{access base class of}|%DerivedClassAccess{access derived class of}|"
+ "%FieldAccess{access field of}|%ArrayElementAccess{access array element of}|"
+ "%PointerArithmetic{perform pointer arithmetic on}|"
+ "%RealComponentAccess{access real component of}|%ImaginaryComponentAccess{access imaginary component of}"
+ "}0 null pointer">;
def note_constexpr_null_callee : Note<
"'%0' evaluates to a null function pointer">;
def note_constexpr_function_param_value_unknown : Note<
@@ -174,16 +178,22 @@ def note_constexpr_this : Note<
"%select{|implicit }0use of 'this' pointer is only allowed within the "
"evaluation of a call to a 'constexpr' member function">;
def access_kind : TextSubstitution<
- "%select{read of|read of|assignment to|increment of|decrement of|"
- "member call on|dynamic_cast of|typeid applied to|construction of|"
- "destruction of|read of}0">;
+ "%enum_select<AccessKind>{"
+ "%ReadOf{read of}|%AssignmentTo{assignment to}|%IncrementOf{increment of}|"
+ "%DecrementOf{decrement of}|%MemberCallOn{member call on}|"
+ "%DynamicCast{dynamic_cast of}|%TypeIdApplied{typeid applied to}|"
+ "%ConstructionOf{construction of}|%DestructionOf{destruction of}|"
+ "%ReadOfAgain{read of}"
+ "}0">;
def access_kind_subobject : TextSubstitution<
- "%select{read of|read of|assignment to|increment of|decrement of|"
- "member call on|dynamic_cast of|typeid applied to|"
- "construction of subobject of|destruction of|read of}0">;
+ "%enum_select<AccessKind>{%ReadOf{read of}|%AssignmentTo{assignment to}|"
+ "%IncrementOf{increment of}|%DecrementOf{decrement of}|"
+ "%MemberCallOn{member call on}|%DynamicCastOf{dynamic_cast of}|"
+ "%TypeidAppliedTo{typeid applied to}|%ConstructionOfSubobject{construction of subobject of}|"
+ "%DestructionOf{destruction of}|%ReadOf{read of}}0">;
def access_kind_volatile : TextSubstitution<
- "%select{read of|read of|assignment to|increment of|decrement of|"
- "<ERROR>|<ERROR>|<ERROR>|<ERROR>|<ERROR>|<ERROR>}0">;
+ "%enum_select<AccessKindVolatile>{%ReadOf{read of}|%AssignmentTo{assignment to}|"
+ "%IncrementOf{increment of}|%DecrementOf{decrement of}|%Error{<ERROR>}}0">;
def note_constexpr_lifetime_ended : Note<
"%sub{access_kind}0 %select{temporary|variable}1 whose "
"%plural{8:storage duration|:lifetime}0 has ended">;
@@ -313,8 +323,8 @@ def note_constexpr_bit_cast_unsupported_bitfield : Note<
"constexpr bit_cast involving bit-field is not yet supported">;
def note_constexpr_bit_cast_invalid_type : Note<
"bit_cast %select{from|to}0 a %select{|type with a }1"
- "%select{union|pointer|member pointer|volatile|reference}2 "
- "%select{type|member}1 is not allowed in a constant expression">;
+ "%enum_select<CastType>{%Union{union}|%Pointer{pointer}|%MemberPointer{member pointer}|"
+ "%Volatile{volatile}|%Reference{reference}}2 is not allowed in a constant expression">;
def note_constexpr_bit_cast_invalid_subtype : Note<
"invalid type %0 is a %select{member|base}1 of %2">;
def note_constexpr_bit_cast_invalid_vector : Note<
@@ -485,7 +495,9 @@ def warn_odr_tag_type_inconsistent
"units">,
InGroup<ODR>;
def note_odr_tag_kind_here: Note<
- "%0 is a %select{struct|interface|union|class|enum}1 here">;
+ "%0 is a %enum_select<ODRTagKind>{"
+ "%Struct{struct}|%Interface{interface}|%Union{union}|"
+ "%Class{class}|%Enum{enum}" "}1 here">;
def note_odr_field : Note<"field %0 has type %1 here">;
def note_odr_field_name : Note<"field has name %0 here">;
def note_odr_missing_field : Note<"no corresponding field here">;
@@ -623,23 +635,25 @@ def note_second_module_difference : Note<
def err_module_odr_violation_definition_data : Error <
"%q0 has different definitions in different modules; first difference is "
"%select{definition in module '%2'|defined here}1 found "
- "%select{"
- "%4 base %plural{1:class|:classes}4|"
- "%4 virtual base %plural{1:class|:classes}4|"
- "%ordinal4 base class with type %5|"
- "%ordinal4 %select{non-virtual|virtual}5 base class %6|"
- "%ordinal4 base class %5 with "
- "%select{public|protected|private|no}6 access specifier}3">;
+ "%enum_select<BaseClassType>{"
+ "%BaseClass{base %plural{1:class|:classes}}|"
+ "%VirtualBase{virtual base %plural{1:class|:classes}}|"
+ "%OrdinalBaseClassWithType{ordinal base class with type %5}|"
+ "%OrdinalVirtualBaseClass{ordinal %select{non-virtual|virtual}5 base class %6}|"
+ "%OrdinalBaseClassWithAccess{ordinal base class %5 with "
+ "%select{public|protected|private|no}6 access specifier}"
+ "}3">;
def note_module_odr_violation_definition_data : Note <
"but in '%0' found "
- "%select{"
- "%2 base %plural{1:class|:classes}2|"
- "%2 virtual base %plural{1:class|:classes}2|"
- "%ordinal2 base class with different type %3|"
- "%ordinal2 %select{non-virtual|virtual}3 base class %4|"
- "%ordinal2 base class %3 with "
- "%select{public|protected|private|no}4 access specifier}1">;
+ "%enum_select<BaseClassType>{"
+ "%BaseClass{base %plural{1:class|:classes}}|"
+ "%VirtualBase{virtual base %plural{1:class|:classes}}|"
+ "%OrdinalBaseClassWithDifferentType{ordinal base class with different type %3}|"
+ "%OrdinalBaseClassWithAccess{ordinal %select{non-virtual|virtual}3 base class %4}|"
+ "%OrdinalBaseClassWithAccessSpecifier{ordinal base class %3 with "
+ "%select{public|protected|private|no}4 access specifier}"
+ "}1">;
def err_module_odr_violation_objc_interface : Error <
"%0 has different definitions in different modules; first difference is "
|
|
|
ayokunle321
changed the title
Awesome, glad to hear it!
5 is a pretty decent heuristic for a first-pass. I would expect though that the USES of these are much more important than the count. The point of
For example: see the ones I changed here: bf17016 All/most of the calls/uses of diagnostics there are just hard coded values, which are pretty unreadable/error-prone.
Sure! Basically "not magic numbers". That is, if the diagnostic-select is always called with some sort of expression that wouldn't be reasonably replaced with the enumerator, it doesn't make sense to change it. BUT if it is always called with an integer constant (or even, passed-to-the-calling function as an integer, even if that integer was derived via logic elsewhere), the enum_select probably makes sense to use.
They currently don't seem to build? But the real way I'd review these is based on how much it improves the readability of the call sites, which you haven't changed. |
| "access real component of|access imaginary component of}0 " | ||
| "pointer past the end of object">; | ||
| "cannot %enum_select<AccessType>{" | ||
| "%BaseClassAccess{access base class of}|%DerivedClassAccess{access derived class of}|" |
There was a problem hiding this comment.
I think these can just be e.g. BaseClass rather than BaseClassAccess, otherwise we’ll end up with AccessKind::BaseClassAccess, which feels a bit redundant.
| "access array element of|perform pointer arithmetic on|" | ||
| "access real component of|" | ||
| "access imaginary component of}0 null pointer">; | ||
| "cannot %enum_select<NullSubobjectAccess>{" |
There was a problem hiding this comment.
Hmm, so NullSubobjectAccess has basically the same enumerators as AccessType above—except that the latter has an additional ERROR case, which I’m candidly not sure what that one is even for, but I also haven’t checked the call site(s) for that diagnostic.
Somewhat relatedly, below we have AccessKind twice two different diagnostics, which currently isn’t supported.
@erichkeane We recently discussed allowing ‘redeclaring’ an %enum_select iff the enumerators and text are the same across all uses of %enum_select with the same enumeration. I think it might additionally make sense to allow just e.g. %enum_select<T>, that is e.g. cannot %enum_select<AccessType>0 null pointer, to reference an existing %enum_select. That should be fairly simple to implement; I finally have some more time on my hands, so I could come up w/ a patch for that later today.
There was a problem hiding this comment.
additionally
Or actually, maybe instead rather than additionally? I.e. don’t allow %enum_select<T>{...} for the same T multiple times, but instead only allow referencing an existing enum_select by writing e.g. just %enum_select<T>0 rather than %enum_select<T>{...}0. That would also save us from having to check whether two definitions of an enum_select are actually the same.
There was a problem hiding this comment.
We recently discussed allowing ‘redeclaring’ an %enum_select iff the enumerators and text are the same across all uses of %enum_select with the same enumeration. I think it might additionally make sense to allow just e.g. %enum_select, that is e.g. cannot %enum_select0 null pointer, to reference an existing %enum_select. That should be fairly simple to implement; I finally have some more time on my hands, so I could come up w/ a patch for that later today.
Hmm... pulling in the same text as well is interesting... What we discussed was pulling in the same %enum_select iif the enumerators were the same across all uses, not if the text itself was. I think ensuring the text is the same isn't necessary, and is actually problematic.
The re-use of the same text like that is interesting.... I'd not thought of doing something like that (without the curleys), but I'm a touch afraid it harms readability of the diagnostic.
There was a problem hiding this comment.
The re-use of the same text like that is interesting.... I'd not thought of doing something like that (without the curleys), but I'm a touch afraid it harms readability of the diagnostic.
I mean, I personally wouldn’t consider
cannot %enum_select<NullSubobjectAccess>{
"%BaseClassAccess{access base class of}|%DerivedClassAccess{access derived class of}|"
"%FieldAccess{access field of}|%ArrayElementAccess{access array element of}|"
"%PointerArithmetic{perform pointer arithmetic on}|"
"%RealComponentAccess{access real component of}|%ImaginaryComponentAccess{access imaginary component of}"
"}0 null pointer
to be more readable than
cannot %enum_select<NullSubobjectAccess>0 null pointer
I’d imagine that in cases where we’d want to reuse the enumeration, it wouldn’t be too uncommon to also want to reuse the text, but I haven’t checked. This was just an idea I had earlier while looking at this pr because here we do have some diagnostics that more or less just share the same text.
There was a problem hiding this comment.
I kind of consider the latter to at least be somewhat more readable? In that I don't have to go find the other declaration to see what those text items are, and modifying them to be different (because it is sensible to have the same Enumerators but different text) ends up being more onerous.
But I can also see the value of just reusing it? I'd be interested to see examples. That said, variants where the enumerators are the same but text is different HAS to be supported.
There was a problem hiding this comment.
I kind of consider the latter to at least be somewhat more readable? In that I don't have to go find the other declaration to see what those text items are, and modifying them to be different (because it is sensible to have the same Enumerators but different text) ends up being more onerous.
I think you meant the former but yeah, I see what you mean.
That said, variants where the enumerators are the same but text is different HAS to be supported.
Yeah, actually, agreed; there definitely are cases where we have related diagnostics with the same cases but different text
There was a problem hiding this comment.
I think you meant the former but yeah, I see what you mean.
Yes, sigh
|
@erichkeane thanks a lot for the clarification. I think I have a hang of it now- I was kinda confused before. Now, my approach is to look through DiagnosticXKinds.td files -> check for a Now I wanted to ask - what do you consider a lot - 3 or more uses with magic numbers? For a Also could you just brief me through the process of a change. I know the .td files are used to generate header files. So do I have to change the select to an enum_select first in the .td files and then build and then reference the generated enum option in the caller (e.g. diag::MemClassWork::AliasDecl) and then rebuild. The thing about this is that the build would probably fail the first time as the callers will still make use of the magic numbers. I also found an enum declared just for a select that was already in a header file. For this case, I just remove this declaration right? And then replace the select. I guess I’m just confused about the whole header generation process - where the new enums would live since the relevant header files already exist before a build (e.g Sema.h, Decl.h) and since we’re making changes to .td files which are the header generators. Kinda new to clang stuff so forgive me for these questions 😅 |
Yeah, that seems like a good strategy.
I don't have a 'hard' number here. When I was looking for a few to demo it with, I just found ones where it was 'most' or 'all' uses.
Another time I don't have a 'hard' number. It is a bit of a balance between 'a lot of work' and 'transform enough of the uses that it improves readability'. If 4 gives a good number of candidates, that is probably a good place to start.
Yes, this would be the first/second step.
Magic numbers will still compile and work. The version of
Yes, you'd want to remove the old ones. Tablegen will create the enums in the file |
ayokunle321
force-pushed
the
refactor-select
branch
from
a39b02e to
9e08ddf
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@erichkeane Hey! So I was able to change one, could you please take a look. Also for this case: Instruction: Use: I was wondering if it was okay for a replacement since the names of the CommandTraits options are the same. Also would a select with only 1 use (with a magic number) be up for replacement? |
I'll check out the review. As far as this CommandTraits one, it probably is not a good candidate. There is already an enum for it that is stored in the AST. As far as 1 use: Yes, if it has 1 use with a magic number, that is a perfectly reasonable thing to replace with an enum_select. |
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
The parts I could see are in teh right direction, I've made some comments.
Please undo the clang-format of ExprConstant.cpp (and only clang-format the parts of the file you modified) and I can look at the hwole thing.
| def note_constexpr_invalid_cast : Note< | ||
| "%select{reinterpret_cast|dynamic_cast|%select{this conversion|cast that" | ||
| " performs the conversions of a reinterpret_cast}1|cast from %1}0" | ||
| "%enum_select<CastKind>{%Reinterpret{reinterpret_cast}|%Dynamic{dynamic_cast}|" |
There was a problem hiding this comment.
| "%enum_select<CastKind>{%Reinterpret{reinterpret_cast}|%Dynamic{dynamic_cast}|" | |
| "%enum_select<CastKind>{%Reinterpret{reinterpret_cast}|%Dynamic{dynamic_cast}|" |
The name CastKind likely should be a little less generic, simply because we can't really duplicate names. Perhaps: ConstexprInvalidCastKind?
| "field %1 within %0 is less aligned than %2 and is usually due to %0 being " | ||
| "packed, which can lead to unaligned accesses">, InGroup<UnalignedAccess>, DefaultIgnore; | ||
| } | ||
| } |
There was a problem hiding this comment.
We still need the newline.
ayokunle321
force-pushed
the
refactor-select
branch
from
73fc237 to
37e6e47
Compare
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
This one looks like a good candiate and the application is much clearer.
There are 2 comments (1 i think the name could be clearer, and the 2nd, we still need a newline at the end fo the file).
Else, let me know how you want to proceed. We can rename this pull request to just rename this ONE (change the PR subject & message) and merge it, or we can wait for you to do quite a few at a time, your choice.
|
Okay cool. I'll change the option's name but for the .td file, I don't think it had a newline at the end initially (just checked the main repo). I could easily add it but just wanted to clarify. And yeah we can merge this one for now and when I'm able to find more I'll make another PR for them. |
Github is showing a change? We need a '\n' after the '}' character (which may be less visible), else this is UB in certain versions of the standard. You can probably do an 'interactive' 'checkout' ( |
|
Latest patch looks right! Just change the Github subject and Commit message to better reflect what this patch is doing, and I can approve. IF that happens, either merge yourself (if you have permissions), or ping me after CI is happy, and I'll merge this for you. |
ayokunle321
changed the title
|
@erichkeane CI is happy now! For future patches, I was wondering if to separate them like this so reviews could be easier or I should just do it all in one PR. |
Separate reviews would be great, I can get those done pretty quickly. |
|
Hey @erichkeane, I'm back! So I'd thought to generalize a bit with the replacements. I saw one with magic numbers in the switch statement just before the diag call and tried to use an enum select there as well. The variable DiagSelect is one-indexed so the default value in the switch steatment is zero. If I change it to use enum_select (which is zero indexed) I would have to change the default value to -1 instead. My question is that why is DiagSelect one-indexed if it's just gonna be subreacted by 1 later on? Is it just legacy to have defaults as 0? And if so, this case should not be up for replacement? 
|
Oh boy! That is an interesting function. DiagSelect there is being used as 1-index because they wanted to use '0' as the 'don't diag' mechanism. It seems to me that making it a |
4 participants
Handles #123121
This patch updates
note_constexpr_invalid_castdiagnostic to useenum_selectinstead ofselect,improving readability and reducing reliance on magic numbers in caller sites.@erichkeane