[PATCH] D88859: APINotes: add APINotesYAMLCompiler

Gabor Marton via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 19 09:49:34 PDT 2020


martong added a comment.

> as it is something which is actually in production already
> ...
> At the very least we need it for compatibility - this is already a shipping feature.

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.



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



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


================
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);
----------------
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>` ?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88859



More information about the cfe-commits mailing list