dang requested changes to this revision.
dang added a comment.
This revision now requires changes to proceed.
You should fix the test to take into account the serializer feedback I left
behind
================
Comment at: clang/include/clang/SymbolGraph/API.h:33
+
+struct APIRecord {
+ StringRef Name;
----------------
Might be worth deleting the default constructor.
================
Comment at: clang/include/clang/SymbolGraph/API.h:44
+ /// Discriminator for LLVM-style RTTI (dyn_cast<> et al.)
+ enum RecordKind {
+ RK_Global,
----------------
APIRecord isn't an abstract class so can be instantiated. To be strictly
compliant with the LLVM-style RTTI you would need an enum case for a plain
record. Otherwise you could introduce a pure virtual function to APIRecord to
make abstract. A good candidate for that would be the destructor, if you make
that pure virtual but provide an empty implementation, the behaviour should be
what you want.
================
Comment at: clang/lib/AST/RawCommentList.cpp:366
+ auto DropTrailingNewLines = [](std::string &Str) {
+ while (!Str.empty() && Str.back() == '\n')
----------------
Consider not using a lambda for this since you only do this once. Also consider
rewriting the logic as
```
auto LastChar = S.find_last_not_of("\n");
S.erase(LastChar+1, S.size())
```
================
Comment at: clang/lib/AST/RawCommentList.cpp:431
}
+ std::string Line;
llvm::StringRef TokText = L.getSpelling(Tok, SourceMgr);
----------------
Might want to use a `SmallString<124>` here as it is likely that the comment
line would fit into that and the `+=` operator would only operate on stack
storage. In the end of the common case (the line fits in 124 chars) you would
only get a single heap allocation when you store emplace it into `Results`.
================
Comment at: clang/lib/SymbolGraph/DeclarationFragments.cpp:54
+ case DeclarationFragments::FragmentKind::GenericParameter:
+ return "genericParam";
+ case DeclarationFragments::FragmentKind::ExternalParam:
----------------
"genericParameter" is what is used in SymbolKit (c.f.
https://github.com/apple/swift-docc-symbolkit/blob/main/Sources/SymbolKit/SymbolGraph/Symbol/DeclarationFragments/Fragment/FragmentKind.swift)
================
Comment at: clang/lib/SymbolGraph/DeclarationFragments.cpp:76
+ DeclarationFragments::FragmentKind::TypeIdentifier)
+ .Case("genericParam",
+ DeclarationFragments::FragmentKind::GenericParameter)
----------------
ditto "genericParameter"
================
Comment at: clang/lib/SymbolGraph/ExtractAPIConsumer.cpp:173
+public:
+ explicit ExtractAPIConsumer(ASTContext &Context,
+ std::unique_ptr<raw_pwrite_stream> OS)
----------------
`explicit` doesn't do anything here since this constructor takes in 2 arguments?
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:123
+ case Language::ObjC:
+ return "objc";
+
----------------
Should be "occ"
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:233
+ case GVKind::Function:
+ Kind["identifier"] = (getLanguageName(LangOpts) + ".function").str();
+ Kind["displayName"] = "Function";
----------------
".func"
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:237
+ case GVKind::Variable:
+ Kind["identifier"] = (getLanguageName(LangOpts) + ".variable").str();
+ Kind["displayName"] = "Variable";
----------------
".variable"
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:238
+ Kind["identifier"] = (getLanguageName(LangOpts) + ".variable").str();
+ Kind["displayName"] = "Variable";
+ break;
----------------
"Global Variable"
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:252
+
+const VersionTuple Serializer::FormatVersion{0, 5, 3};
+
----------------
Currently the spec
(https://github.com/apple/swift-docc-symbolkit/blob/main/openapi.yaml) lists
0.3.0 as the version. Let's use that.
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:258
+ serializeSemanticVersion(FormatVersion));
+ Metadata["generator"] = "clang";
+ return Metadata;
----------------
something more descriptive would be nice here. I think you want the output off
`getClangFullRepositoryVersion` or `getClangFullVersion` so we have something
meaningful to go off for future bugs.
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:264
+ Object Module;
+ // FIXME: What to put in here?
+ Module["name"] = "";
----------------
As discussed offline we need to pass clang an extra flag to populate this. I
suggest `--product-name`.
================
Comment at: clang/lib/SymbolGraph/Serialization.cpp:283
+ serializeObject(Obj, "docComment", serializeDocComment(Record.Comment));
+ serializeArray(Obj, "declaration",
+ serializeDeclarationFragments(Record.Declaration));
----------------
This is called "declarationFragments" also this is a function only concept as
per the spec
https://github.com/apple/swift-docc-symbolkit/blob/224736ddcdfcba87ae92bb5c8d74e8332f218f36/openapi.yaml#L311
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119479/new/
https://reviews.llvm.org/D119479
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
