martong added inline comments.

================
Comment at: clang/include/clang/APINotes/Types.h:25
+/// auditing.
+enum class EnumExtensibilityKind {
+  None,
----------------
compnerd wrote:
> 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.
Thanks for clarifying this. It was not clear for me that these types were to 
represent the YAML file contents. It makes much more sense now.

> Is there a good place to write this down?
I think a file comment right after the license could do it.


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

Reply via email to