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