[PATCH] D88859: APINotes: add APINotesYAMLCompiler

[Gabor Marton via Phabricator via cfe-commits]

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