martong added a comment.
In D88859#2359399 <https://reviews.llvm.org/D88859#2359399>, @compnerd wrote:
>> I'd like to have maximum flexibility from the submitter by willing to change
>> the details of the implementation
>
> I don't think that is a fair expectation - there are plenty of times where
> this is technical disagreement on functionality, and it doesn't get
> immediately resolved. A high quality implementation doesn't require that
> everything be changed immediately, nor does it mean that everything must be
> completely flexible. There are trade-offs, and the goal is to strike a
> balance between simplicity and the needs of the project.
Yes, absolutely, flexibility is needed from both sides (submitter and
reviewer). It is our common interest to get an extensible implementation
upstreamed. I hope I didn't push it too hard, I didn't mean any offence.
================
Comment at: clang/include/clang/APINotes/Types.h:25
+/// auditing.
+enum class EnumExtensibilityKind {
+ None,
----------------
compnerd wrote:
> martong wrote:
> > compnerd wrote:
> > > martong wrote:
> > > > This seems a bit redundant to `Attrs.td`. I'd prefer to have the
> > > > structure of an attribute defined only in one place. Why can't we
> > > > directly reuse the types in the generated `Attrs.inc`? In this case
> > > > ```
> > > > class EnumExtensibilityAttr : public InheritableAttr {
> > > > public:
> > > > enum Kind {
> > > > Closed,
> > > > Open
> > > > };
> > > > ```
> > > > could perfectly fit, wouldn't it?
> > > The none-case here is not the same as missing - it tracks the explicitly
> > > audited case. I suppose we can change the internal enum case from `None`
> > > to `Audited` if you like.
> > I am not sure I can follow. Could you please elaborate what is an "explicit
> > y audited" case?
> > https://clang.llvm.org/docs/AttributeReference.html#enum-extensibility
> > mentions only open/closed ...
> There are three states consider:
>
> ```
> enum Unaudited {
> };
> enum __attribute__((__enum_extensibility__(open))) Open {
> };
> enum __attribute__((__enum_extensibility__(closed))) Closed {
> };
> ```
>
> The optionality of the value indicates whether the value is present or not in
> the APINotes, not the tri-state nature of the attribute.
Ok. Thanks for the explanation.
But how could we describe then `flag_enum`? As a possible extension in the
future (not in this patch).
E.g.:
```
enum __attribute__((enum_extensibility(open),flag_enum)) OpenFlagEnum {
D0 = 1 << 0, D1 = 1 << 1
};
```
Or here
```
enum __attribute__((flag_enum)) Foo;
```
should `Foo` have an `Audited` or `None` `Kind` here?
If we were to add support to `flag_enum` would you create a new enum class for
that here?
Note that I am asking all these questions because I am planning to extend and
add more attributes in the future once the whole APINotes stuff is landed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88859/new/
https://reviews.llvm.org/D88859
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
