[PATCH] D88859: APINotes: add APINotesYAMLCompiler

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.


================
Comment at: clang/include/clang/APINotes/Types.h:25
+/// auditing.
+enum class EnumExtensibilityKind {
+  None,
----------------
martong wrote:
> 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.
The questions are reasonable - and we should understand what is missing and what can be done and what cannot be done.

I think that the use of the metadata is extremely important.  The question is, are there any users who care about recovering that information?  IMO, we should try to represent the input as faithfully as possible, even if that means that we need to replicate the enumerations.  However, if the consumers do not care about the inferred state vs the written state, I don't think that it is an immediate concern if we cannot recover that from the representation, we can simplify and extend as and when needed, especially when we have the feature merged (I think that it is easier once merged only because this is a large feature, and there is a large user base that is using this, which complicates things.  Really, this functionality should have been merged a long time ago, but that is both my own opinion and past), and hopefully in many cases we can do it during review time.

The tricky bit to remember that there is an additional state that is implicitly being added here, and optionality should reference the contents of the APINotes, not the state in the declaration that is being mutated.

Is there a good place to write this down? I am willing to add comments explaining this so that the next person (ha, no, not the next person, but me, because I *will* forget) is not tripped up on this nuance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88859/new/

https://reviews.llvm.org/D88859