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