martong added a comment. Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html
================ Comment at: clang/include/clang/APINotes/APINotesYAMLCompiler.h:16 +namespace api_notes { +bool parseAndDumpAPINotes(llvm::StringRef YI); +} ---------------- It would be useful to have more documentation here. ================ Comment at: clang/include/clang/APINotes/Types.h:1 +//===-- Types.h - API Notes Data Types --------------------------*- C++ -*-===// +// ---------------- 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). ================ Comment at: clang/include/clang/APINotes/Types.h:25 +/// auditing. +enum class EnumExtensibilityKind { + None, ---------------- This seems a bit redundant to `Attrs.td`. I'd prefer to have the structure of an attribute defined only in one place. Why can't we directly reuse the types in the generated `Attrs.inc`? In this case ``` class EnumExtensibilityAttr : public InheritableAttr { public: enum Kind { Closed, Open }; ``` could perfectly fit, wouldn't it? ================ Comment at: clang/include/clang/APINotes/Types.h:32 +/// The kind of a swift_wrapper/swift_newtype. +enum class SwiftWrapperKind { + None, ---------------- Should this be rather `SwiftNewTypeKind`? Since `swift_wrapper` is deprecated according to the docs: https://clang.llvm.org/docs/AttributeReference.html#swift-newtype ================ Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:240 +namespace { +struct Class { + StringRef Name; ---------------- 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? ================ 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); ---------------- Hmm, why do we need "none"? Can't we interpret the non-existence as "none"? ================ Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:621 + + llvm::yaml::Output OS(llvm::outs()); + OS << M; ---------------- 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? ================ Comment at: clang/test/APINotes/Inputs/Frameworks/Simple.framework/Headers/Simple.apinotes:1 +Name: SimpleKit +Classes: ---------------- 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. ================ Comment at: clang/test/APINotes/yaml-roundtrip.test:4 + +We expect only the nullability to be different as it is canonicalized during the +roudtrip. ---------------- Why do we have this difference? This seems odd and as a superfluous complexity. ================ Comment at: clang/tools/apinotes-test/APINotesTest.cpp:22 + + for (const auto &Notes : APINotes) { + llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> NotesOrError = ---------------- `auto` -> `std::string` 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