I noticed PropertyAttributeKind / ObjCPropertyAttributeKind enums in ObjCPropertyDecl and ObjCDeclSpec are exactly identical. This is a non-functional change to make these two enums less redundant.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/AST/DeclObjCCommon.h | ||
---|---|---|
22 | It seems that you are touching all the sites that use this, so perhaps this is the time to change this to enum class and drop the OBJC_PR_ prefixes and explicitly inherit from uint16_t. This should be named ObjCPropertyAttribute I think. |
clang/include/clang/AST/DeclObjCCommon.h | ||
---|---|---|
22 | Ah yeah, that sounds pretty good. Will do. |
clang/include/clang/AST/DeclObjCCommon.h | ||
---|---|---|
22 | Talked offline: update is that its not so easy to change these enums to enum classes because of how they are constantly used with unsigned ints. I could try and implement lots of operator overloads but I think that could be potentially buggy and not so NFC-like. @compnerd you wanted to leave the OBJC_PR_ and dropped the DQ prefixes right? Other than that, ping (for others)? I think this could be a nice cleanup. |
clang/include/clang/AST/DeclObjCCommon.h | ||
---|---|---|
22 | If you just want the scoping benefits of the enum class you can simulate it with a namespace: namespace ObjCPropertyAttribute { enum { noattr, readonly, ... }; } Which would make the users of this enum look a bit nicer. I agree that adding overloading operators is over-engineering this. |
clang/include/clang/AST/DeclObjCCommon.h | ||
---|---|---|
22 | Ah yeah, nice. so the prefixes can be dropped. Another reason I don't want to change the enum to an enun class is, from the comments it seems a lot of the use of unsigned is to avoid the differences in how Win32 vs *nix handle enums (signed vs unsigned I think). I'll post a follow up with the scoping. |
Move ObjCPropertyAttributeKind to namespace ObjCPropertyAttribute { enum Kind { ... }}
LGTM (after fixing those tests). Thanks for cleaning this up!
clang/tools/c-index-test/c-index-test.c | ||
---|---|---|
1107 ↗ | (On Diff #256931) | Hm, looks like this change is breaking some CHECK lines in the indexer tests. |
clang/include/clang-c/Index.h | ||
---|---|---|
4503 ↗ | (On Diff #256931) | @erik.pilkington Do you think we should be adding the C-API analogs of nullability / null_resettable (and direct) to clang/include/clang-c/Index.h? I noticed those are missing. |
clang/include/clang-c/Index.h | ||
---|---|---|
4503 ↗ | (On Diff #256931) | @erik.pilkington @arphaman Any chance I can land the C API changes in a follow up commit? That's what I was thinking. |
clang/include/clang-c/Index.h | ||
---|---|---|
4503 ↗ | (On Diff #256931) | I think this enumerator should stay named CXObjCPropertyAttr_class, changing the name to CXObjCPropertyAttr_classattr is API breaking, and the C API should be stable. |
clang/include/clang-c/Index.h | ||
---|---|---|
4503 ↗ | (On Diff #256931) | Ah yeah! Good catch. Makes total sense. What do you think about dropping the C macro usage in clang/tools/c-index-test/c-index-test.c (ie PRINT_PROP_ATTR)? I can't drop the prefix in the c++ enum completely without changing class to classattr since it is a keyword. |
llvm/include/llvm/BinaryFormat/Dwarf.def | ||
---|---|---|
916 | I have some concerns here too. How would this affect the Dwarf apple property? |
Adding a kind_ prefix to avoid any keywords being used in the ObjCPropertyAttribute enum.
Can this be a LGTM again @erik.pilkington @arphaman? I have managed to undo any unintended C API changes. Tests appear to all pass too. I had to add a short prefix to the enum values to avoid hitting any C++ keywords (ie class). Is there anything else needed for this refactor to go in?
It seems that you are touching all the sites that use this, so perhaps this is the time to change this to enum class and drop the OBJC_PR_ prefixes and explicitly inherit from uint16_t. This should be named ObjCPropertyAttribute I think.