[PATCH] D88859: APINotes: add APINotesYAMLCompiler

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Thanks for the feedback!



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


================
Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:240
+namespace {
+struct Class {
+  StringRef Name;
----------------
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`.


================
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:
> 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.


================
Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:621
+
+  llvm::yaml::Output OS(llvm::outs());
+  OS << M;
----------------
martong wrote:
> I think the stream (`llvm::outs`) should not be written in stone. And why not `llvm::errs` (we `dump` to `errs` usually) ? Could it be a parameter?
Sure, that is reasonable, I should be able to add a `llvm::raw_ostream &` parameter.


================
Comment at: clang/test/APINotes/Inputs/Frameworks/Simple.framework/Headers/Simple.apinotes:1
+Name:            SimpleKit
+Classes:
----------------
martong wrote:
> I am pretty sure this does not provide a full coverage. What about e.g `Functions`? I'd like to see them tested as well.
Correct, it isn't meant to provide full coverage at all.  It is merely meant to be enough to ensure that we can load the YAML and process it.  The full coverage will come with the follow up patches as they will require filling out more of the infrastructure.  I am trying to keep the patch at a reasonable size.  I can add additional test cases if you feel strongly that they should be added now, but, I do worry about the size of the patch ballooning.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88859