[PATCH] D88859: APINotes: add APINotesYAMLCompiler

Mon Oct 26 11:37:59 PDT 2020

compnerd marked 3 inline comments as done.
compnerd added a comment.

In D88859#2339159 <https://reviews.llvm.org/D88859#2339159>, @martong wrote:

> I value that you guys have a prototype on a fork that is being used in production. But, upstreaming and these reviews give us the chance to evolve and to create an implementation that serves well the //whole// Clang community. 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. 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'm not against evolving the implementation.  However, compatibility with production code is important.  I don't think that it is possible to ignore that, especially when the impact is non-trivial (it is not a small localized use).  The large scale usage is valuable in terms of demonstrating the value of the functionality, so it is valuable to clang community.  I am not suggesting just a copy from the Swift implementation, and do care about it being a high quality implementation.  But that cannot come at the cost of compatibility with the existing usage.

================
Comment at: clang/include/clang/APINotes/Types.h:1
+//===-- Types.h - API Notes Data Types --------------------------*- C++ -*-===//
+//
----------------
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?

================
Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:240
+namespace {
+struct Class {
+  StringRef Name;
----------------
martong wrote:
> compnerd wrote:
> > martong wrote:
> > > Why are these classes in a unnamed namespace? I'd expect them to be in the header under the `clang::api_notes` namespace, so users of the APINotesYAMLCompiler could use these as the structured form of the YAML content. Isn't that the point of the whole stuff?
> > There will be follow up types that provide structured access to the data.  These types are strictly for the serialization to and from YAML via `YAML::IO`.
> Sounds good. Could you please describe in a nutshell which are the following steps for the upstreaming? I mean it could make the review easier if we could see a kind of road-map for the upcoming patches.
I am still digesting the complete implementation, which is extremely large.

Currently, my thought is to split up it up with this change which only deals with the YAML aspect, then follow that up with the processing that is required:
- conversion to bitcode
- conversion from bitcode
And then the work to associate the attributes from the deserialized form to the AST.

I am trying to split this up into smaller chunks so that it can be adequately reviewed rather than just be a dump of a feature and then have to rework it to be something that makes sense in the upstream tree.

I also feel uncomfortable with the current version in Swift being merged as I can't seem to understand the testing aspect well enough, which is why I introduced the `apinotes-test-tool` so that we can be sure that the pieces can be tested.

================
Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:439
+  static void enumeration(IO &IO, EnumExtensibilityKind &EEK) {
+    IO.enumCase(EEK, "none",   EnumExtensibilityKind::None);
+    IO.enumCase(EEK, "open",   EnumExtensibilityKind::Open);
----------------
martong wrote:
> compnerd wrote:
> > martong wrote:
> > > Hmm, why do we need "none"? Can't we interpret the non-existence as "none"?
> > At the very least we need it for compatibility - this is already a shipping feature.  However, nullability is also not completely annotated.  So, there is some benefit in tracking the explicit none vs missing.
> `Optional<EnumExtensibilityAttr::Kind>` ?
That representation could work, let me see if I can get `YAML::IO` to provide something which would be compatible.

================
Comment at: clang/test/APINotes/yaml-roundtrip.test:4
+
+We expect only the nullability to be different as it is canonicalized during the
+roudtrip.
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88859