[PATCH] D88859: APINotes: add APINotesYAMLCompiler

[Gabor Marton via Phabricator via cfe-commits]

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