[PATCH] D88859: APINotes: add APINotesYAMLCompiler

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D88859#2354377 <https://reviews.llvm.org/D88859#2354377>, @gribozavr2 wrote:

>> I am having a hard time to accept "this is how it is implemented in our fork" as a technical argument. Besides, I am not sure how could the Clang community benefit about being backward compatible with a specialized fork and thus making superfluous complications.
>
> Being compatible with Apple's SDKs is quite important for some part of the community.
>
> This is similar to Clang implementing GCC and MSVC attributes, for example. Some might call them specialized attributes -- they are non-standard and are certainly not used by every C++ user -- but there exists a sizeable population of downstream consumers that rely on them. Apple's SDKs are in the same class.

Fair enough. But I don't think that Clang developers just copied the implementation from GCC or from MSVC. 
I accept to keep the interface of the feature (i.e. the YAML format) as it is in the fork, okay. But, I'd like to have maximum flexibility from the submitter by willing to change the details of the implementation. I don't think that the implementation in the fork is necessarily the best realization, we can do better, after all that's the purpose of this review, isn't it?

>> This feature could be really valuable, but I'd like to have it landed in a high quality form which serves the whole community (and the static analyzer developers). I think that a simple copy-paste of the fork will not do it.
>
> I completely agree that the feature should be useful for static analysis. Do compatibility aliases hurt these goals?

No. But they make the tests and the whole system more complicated, IMHO.



================
Comment at: clang/include/clang/APINotes/Types.h:1
+//===-- Types.h - API Notes Data Types --------------------------*- C++ -*-===//
+//
----------------
compnerd wrote:
> martong wrote:
> > compnerd wrote:
> > > martong wrote:
> > > > So we are mapping existing attributes here, I am missing this to be documented. Why do we support only these attributes?
> > > > Would be consistent to put `SwiftPrivate` here as well, instead of implicitly interpreting that as `bool` in `APINotesYAMLCompiler.cpp`, wouldn't it?
> > > > I think we should list all processed attributes here, or we should just use `Attrs.inc` (see below my comment as well).
> > > These are the ones that are currently needed and can be safely merged.  I really wouldn't want to extend the support to all the attributes in the initial attempt to merge the functionality as it is something which is actually in production already.  However, once the functionality is in a shared state, it is much easier to revise and co-evolve other consumers.
> > I understand that an initial implementation may not support all attributes. What's more, an evolved implementation may not do that either. 
> > 
> > Including `Attrs.inc` does not put any requirement to support all attributes, but we could reuse the types and enums there. This way we could avoid the need to mirror the types in `Attrs.inc`.
> > 
> From that description, I think I am not necessarily following what you are suggesting.  Could you be a bit more concrete in what you are suggesting in terms of the reuse?
I was thinking about to simply
`#include "clang/AST/Attr.h`
(which further includes `Attrs.inc`).

This way the class `clang::EnumExtensibilityAttr` and many other attribute classes are immediately available for your use. And you don't have to define very similar classes here again (unless I misunderstand something).


================
Comment at: clang/include/clang/APINotes/Types.h:25
+/// auditing.
+enum class EnumExtensibilityKind {
+  None,
----------------
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 ...


================
Comment at: clang/test/APINotes/yaml-roundtrip.test:4
+
+We expect only the nullability to be different as it is canonicalized during the
+roudtrip.
----------------
compnerd wrote:
> martong wrote:
> > compnerd wrote:
> > > martong wrote:
> > > > Why do we have this difference? This seems odd and as a superfluous complexity.
> > > The difference is needed for compatibility.  Richard wanted the fully spelt out names, but that requires backwards compatibility, so we need the difference here.
> > I have concerns about making things more complicated just to be compatible with a downstream fork.
> The compatibility is extremely important unfortunately.  We could go with just the terse version for the time being if you are strongly opposed to the two spellings.
I'd rather stick with the terse version if possible (do we have to pursue rsmith about this?) If this is really a hassle then let's forget about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88859