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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
