This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.
ClosedPublic

Authored by plotfi on Apr 1 2020, 11:18 AM.

Details

Summary

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.

Diff Detail

Event Timeline

plotfi created this revision.Apr 1 2020, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 11:18 AM
plotfi edited the summary of this revision. (Show Details)Apr 1 2020, 11:18 AM
plotfi updated this revision to Diff 254259.Apr 1 2020, 11:21 AM
This comment was removed by plotfi.
plotfi added a reviewer: jfb.Apr 1 2020, 11:23 AM
plotfi updated this revision to Diff 254297.Apr 1 2020, 1:37 PM

Applying clang-format suggestions.

compnerd added inline comments.Apr 1 2020, 9:35 PM
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.

plotfi marked an inline comment as done.Apr 1 2020, 10:03 PM
plotfi added inline comments.
clang/include/clang/AST/DeclObjCCommon.h
22

Ah yeah, that sounds pretty good. Will do.

plotfi marked 2 inline comments as done.Apr 12 2020, 4:03 PM
plotfi added inline comments.
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.

plotfi marked an inline comment as done.Apr 12 2020, 7:58 PM
plotfi added inline comments.
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.

plotfi updated this revision to Diff 256931.EditedApr 12 2020, 11:44 PM

Move ObjCPropertyAttributeKind to namespace ObjCPropertyAttribute { enum Kind { ... }}

plotfi marked an inline comment as done.Apr 12 2020, 11:47 PM
erik.pilkington accepted this revision.Apr 13 2020, 6:09 AM

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.

This revision is now accepted and ready to land.Apr 13 2020, 6:09 AM
plotfi added inline comments.Apr 13 2020, 7:53 AM
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.

erik.pilkington requested changes to this revision.Apr 13 2020, 8:17 AM
erik.pilkington added inline comments.
clang/include/clang-c/Index.h
4503 ↗(On Diff #256931)

Oh, sorry, I missed this. The C API is supposed to be stable, so I don't think we should change the name of this enumerator (@arphaman can you confirm?). Adding the missing attributes seem fine though.

This revision now requires changes to proceed.Apr 13 2020, 8:17 AM
plotfi marked an inline comment as done.Apr 13 2020, 9:21 AM
plotfi added inline comments.
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.

plotfi updated this revision to Diff 257207.Apr 13 2020, 11:51 PM

adding fixes for class -> classattr change for tests

plotfi marked an inline comment as done.Apr 14 2020, 12:17 AM
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.

plotfi marked an inline comment as done.Apr 14 2020, 10:51 AM
plotfi added inline comments.
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.

plotfi updated this revision to Diff 257431.Apr 14 2020, 11:52 AM

Removing unintended change to Clang C API

plotfi marked an inline comment as done.Apr 14 2020, 2:11 PM
plotfi added inline comments.
llvm/include/llvm/BinaryFormat/Dwarf.def
916

I have some concerns here too. How would this affect the Dwarf apple property?

plotfi updated this revision to Diff 257550.Apr 14 2020, 4:07 PM

Adding a kind_ prefix to avoid any keywords being used in the ObjCPropertyAttribute enum.

plotfi updated this revision to Diff 257577.Apr 14 2020, 5:52 PM

Update for clang-format changes.

LGTM (after fixing those tests). Thanks for cleaning this up!

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?

erik.pilkington accepted this revision.Apr 22 2020, 10:10 AM

LGTM, again :) Thanks for cleaning this up.

This revision is now accepted and ready to land.Apr 22 2020, 10:10 AM

LGTM, again :) Thanks for cleaning this up.

Thank you Erik!

This revision was automatically updated to reflect the committed changes.
plotfi updated this revision to Diff 259486.Apr 22 2020, 11:27 PM

Including lldb lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp change

Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 11:27 PM
plotfi updated this revision to Diff 259490.Apr 23 2020, 12:36 AM

clang-format to the lldb change