[PATCH] D139115: [clang][ExtractAPI] Add support for single symbol SGF

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added subscribers: akyrtzi, benlangmuir.
benlangmuir added inline comments.


================
Comment at: clang/include/clang-c/Documentation.h:549
 
+typedef struct CXAPISetImpl *CXAPISet;
+
----------------
@dang please document what this type represents.

@akyrtzi what's the preferred implementation type name for opaque typedef types? I see we have a mix right now:
* `void *`
* `CXOpaqueFoo *`
* `CXFooImpl *`


================
Comment at: clang/include/clang-c/Documentation.h:551
+
+CINDEX_LINKAGE enum CXErrorCode clang_createAPISet(CXTranslationUnit tu,
+                                                   CXAPISet *out_api);
----------------
Please add documentation comments to all the new functions. For this one, we should explicitly include
* That the user needs to call `clang_disposeAPISet` for the output
* Whether `out_api` is modified on failure -- e.g. is it null?


================
Comment at: clang/include/clang-c/Documentation.h:556
+
+CINDEX_LINKAGE CXString clang_getSingleSymbolSGFForUSR(const char *usr,
+                                                       CXAPISet api);
----------------
Does SGF stand for "Symbol Graph Fragment"?  How well-known is this abbreviation?  And how commonly-used do you expect these functions to be?  My gut reaction would be to use the full name here.

We should document the null return value for unknown symbols.  Are there other ways this could fail?  Is it ever interesting to know why this failed, or is success/fail always good enough?   Same applies to the function below.


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:40
+
+void WalkupFromMostDerivedType(ExtractAPIVisitor &Visitor, Decl *D);
+
----------------
Nit: we use `static` for global functions and only use anon namespaces for types.  Same for the below.


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:73
+enum CXErrorCode clang_createAPISet(CXTranslationUnit tu, CXAPISet *out_api) {
+  ASTUnit *Unit = cxtu::getASTUnit(tu);
+
----------------
```
  if (isNotUsableTU(tu) || !out_api)
    return CXError_InvalidArguments;
```


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:92
+
+CINDEX_LINKAGE CXString clang_getSingleSymbolSGFForUSR(const char *usr,
+                                                       CXAPISet api) {
----------------
We don't want `CINDEX_LINKAGE` in the implementation, only the header needs it.  Same for the other functions


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:97
+  if (auto SGF = SymbolGraphSerializer::serializeSingleSymbolSGF(usr, *API)) {
+    std::string BackingString;
+    llvm::raw_string_ostream OS(BackingString);
----------------
Nit: maybe `SmallString` and `raw_svector_ostream`?


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:108
+CINDEX_LINKAGE CXString clang_getSingleSymbolSGF(CXCursor cursor) {
+  const CXCursorKind &Kind = clang_getCursorKind(cursor);
+  if (clang_isDeclaration(Kind)) {
----------------
This is binding a temporary; you probably want `CXCursorKind` by value.


================
Comment at: clang/tools/libclang/CXExtractAPI.cpp:129
+    SmallString<128> USR;
+    index::generateUSRForDecl(D, USR);
+
----------------
Probably should check the return value here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139115/new/

https://reviews.llvm.org/D139115