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