[NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.

[plotfi]

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.

[plotfi]

This comment was removed by plotfi.

[plotfi]

Applying clang-format suggestions.

[compnerd]

clang/include/clang/AST/DeclObjCCommon.h

21 ↗

(On Diff #254297)

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]

clang/include/clang/AST/DeclObjCCommon.h

21 ↗

(On Diff #254297)

Ah yeah, that sounds pretty good. Will do.

[plotfi]

clang/include/clang/AST/DeclObjCCommon.h

21 ↗

(On Diff #254297)

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.

[erik.pilkington]

clang/include/clang/AST/DeclObjCCommon.h

21 ↗

(On Diff #254297)

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]

clang/include/clang/AST/DeclObjCCommon.h

21 ↗

(On Diff #254297)

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]

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

[erik.pilkington]

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.

[plotfi]

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]

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.

[plotfi]

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]

adding fixes for class -> classattr change for tests

[erik.pilkington]

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]

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]

Removing unintended change to Clang C API

[plotfi]

llvm/include/llvm/BinaryFormat/Dwarf.def

915 ↗

(On Diff #257431)

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

[plotfi]

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

[plotfi]

Update for clang-format changes.

[plotfi]

Ping

[plotfi]

In D77233#1977671, @erik.pilkington wrote:

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?

[plotfi]

@erik.pilkington @arphaman ping.

[erik.pilkington]

LGTM, again :) Thanks for cleaning this up.

[plotfi]

In D77233#1997294, @erik.pilkington wrote:

LGTM, again :) Thanks for cleaning this up.

Thank you Erik!

[plotfi]

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

[plotfi]

clang-format to the lldb change

clang/tools/libclang/CIndex.cpp

 }
 
 unsigned clang_Cursor_getObjCPropertyAttributes(CXCursor C, unsigned reserved) {
   if (C.kind != CXCursor_ObjCPropertyDecl)
     return CXObjCPropertyAttr_noattr;
 
   unsigned Result = CXObjCPropertyAttr_noattr;
   const ObjCPropertyDecl *PD = dyn_cast<ObjCPropertyDecl>(getCursorDecl(C));
-  ObjCPropertyDecl::PropertyAttributeKind Attr =
-      PD->getPropertyAttributesAsWritten();
+  ObjCPropertyAttributeKind Attr = PD->getPropertyAttributesAsWritten();
 
 #define SET_CXOBJCPROP_ATTR(A)                                                 \
-  if (Attr & ObjCPropertyDecl::OBJC_PR_##A)                                    \
+  if (Attr & ObjCPropertyAttributeKind::OBJC_PR_##A)                           \
   Result |= CXObjCPropertyAttr_##A
   SET_CXOBJCPROP_ATTR(readonly);
   SET_CXOBJCPROP_ATTR(getter);
   SET_CXOBJCPROP_ATTR(assign);
   SET_CXOBJCPROP_ATTR(readwrite);
   SET_CXOBJCPROP_ATTR(retain);
   SET_CXOBJCPROP_ATTR(copy);
   SET_CXOBJCPROP_ATTR(nonatomic);