[PATCH] D88859: APINotes: add APINotesYAMLCompiler
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 16 05:55:26 PDT 2020
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
More information about the cfe-commits
mailing list