plotfi marked an inline comment as done.
plotfi added inline comments.
================
Comment at: clang/include/clang/AST/DeclObjCCommon.h:21
+/// Keep this list in sync with LLVM's Dwarf.h ApplePropertyAttributes.
+enum ObjCPropertyAttributeKind {
+ OBJC_PR_noattr = 0x00,
----------------
erik.pilkington wrote:
> plotfi wrote:
> > plotfi wrote:
> > > compnerd wrote:
> > > > 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.
> > > Ah yeah, that sounds pretty good. Will do.
> > 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.
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77233/new/
https://reviews.llvm.org/D77233
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
