[PATCH] D135804: [clang][ExtractAPI] Ignore fully anonymous RecordDecls

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

It would be ideal to be able to properly handle nested types like this, but for 
right now this is causing a crash in Swift-DocC, so this patch will at least 
get that working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135804

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125678: [clang][extract-api] Don't emit symbols prefixed with an underscore

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added a comment.

clang-format failed:

  ---  clang-format
  
  changed files:
  
  clang/test/ExtractAPI/underscored.c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125678

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125678: [clang][extract-api] Don't emit symbols prefixed with an underscore

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125678

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123391: [clang][extract-api] Emit "navigator" property of "name" in SymbolGraph

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

Looks great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123391

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123391: [clang][extract-api] Emit "navigator" property of "name" in SymbolGraph

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added a comment.

This looks good! Just one question.




Comment at: clang/test/ExtractAPI/objc_interface.m:198
+"kind": "identifier",
+"spelling": "getWithProperty:"
+  }

I'm curious: Does this properly handle Objective-C methods with multiple 
arguments? i.e. if there were a method `setProperty:andOtherProperty:`, would 
that be the navigator name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123391

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123259: [clang][ExtractAPI] Fix appendSpace in DeclarationFragments

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123259

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123261: [clang][ExtractAPI] Fix declaration fragments for ObjC methods

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123261

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123045: [clang][extract-api] Fix small issues with SymbolGraphSerializer

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus accepted this revision.
QuietMisdreavus added a comment.
This revision is now accepted and ready to land.

I like the idea of using the RAII context/guard to manage the path components 
stack. I have one non-blocking comment about the rest of the patch now.




Comment at: 
clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h:125
   /// containing common symbol information of \p Record.
-  Optional serializeAPIRecord(const APIRecord ) const;
+  Optional serializeAPIRecord(const APIRecord );
 

Now that the path components are being manipulated outside of 
`serializeAPIRecord`, can this function become `const` again?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123045

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123045: [clang][extract-api] Fix small issues with SymbolGraphSerializer

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added inline comments.



Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:510
   Symbols.emplace_back(std::move(*Obj));
+  PathComponentContext.pop_back();
 }

dang wrote:
> dang wrote:
> > zixuw wrote:
> > > zixuw wrote:
> > > > QuietMisdreavus wrote:
> > > > > zixuw wrote:
> > > > > > What's the cost/would it worth it to wrap the `emplace_back`s and 
> > > > > > `pop_back`s of `PathComponentContext` in meaningful APIs like 
> > > > > > `enterPathComponentContext` and `exitPathComponentContext`?
> > > > > > That way the code is more self-explanatory and easier to read. It 
> > > > > > took me some time and mental resources to figure out why the 
> > > > > > `pop_back` is placed here.
> > > > > What's the use of having the `emplace_back` call inside 
> > > > > `serializeAPIRecord` but to pop it outside? It seems like it's easier 
> > > > > to mess up for new kinds of records.
> > > > The reason to `emplace_back` the path component inside 
> > > > `serializeAPIRecord` is that the `pathComponent` field of the `Record` 
> > > > is serialized in there so you want to include the name of the symbol 
> > > > itself in the path component list.
> > > > The `pop_back` is delayed til all other additional serialization for a 
> > > > specific kind of record, for example `serializeEnumRecord` handles all 
> > > > the enum cases after processing the enum record itself using 
> > > > `serializeAPIRecord`. So in order to correctly include the enum name in 
> > > > the path components of all the enum cases, the enum name has to stay in 
> > > > `PathComponentContext` until all members are serialized.
> > > > 
> > > > This is exactly the reason why I wanted a clearer API to make it easier 
> > > > to see.
> > > Hmm now that I thought about this, it seems that it would be easier to 
> > > understand and avoid bugs if we lift 
> > > `PathComponentContext.emplace_back`/`enterPathComponentContext` out of 
> > > `serializeAPIRecord`, because we have access to the record name before 
> > > that anyway.
> > > 
> > > So we establish a pre-condition of `serializeAPIRecord` that the correct 
> > > path components would be set up in `PathComponentContext` before the call 
> > > so we could still serialize the field inside that method. And in specific 
> > > `serialize*Record` methods we push the record name, and pop out at the 
> > > end.
> > > 
> > > This way the push and pop operations would appear in pairs in a single 
> > > block, saving the confusion and mental work of jumping across functions 
> > > to see how `PathComponentContext` is evolving.
> > If you think we should have a specialized API I am happy to do this. I 
> > figured it was self-explanatory by the name of `PathComponentContext` but 
> > it clearly isn't so needs addressing. I put the `emplace_back` call in 
> > `serializeAPIRecord` since all the specific serialization methods call it. 
> > I thought it would make it impossible to forget to add them. @zixuw is 
> > correct in the reason why the pop is outside we don't want to pop before we 
> > have serialized the sub records by agree it is ugly and potentially error 
> > prone. I can see three ways forward for improving the ergonomics of this 
> > since this seems to be problematic:
> > - Provide `serializeAPIRecord` a continuation to serialize the sub records 
> > or additional fields, that way we can move the pop inside 
> > `serializeAPIRecord` I am not a fan personally because we get into JS style 
> > closure hell if we start doing this...
> > - Use a visitor pattern where the visitor would be responsible for managing 
> > `PathComponentContext` and do the appropriate push/pops in the `visit` 
> > methods. We would need to add a APIRecordVisitor type, and appropriate 
> > visit methods for each APIRecord. This would make sense because the records 
> > should really know how to visit themselves.
> > - Just add a specialized API although it seems it would really easy to 
> > forget to remove path components.
> > 
> > Let me know what you think is the way forward.
> Unless we go with the last option, I think this should be a follow up patch 
> since it would a structural change.
I'm most a fan of the `APIRecordVisitor` option, but you're right in that that 
would be a rather involved change. Now that you've said that the arrangement is 
for encoding sub-records, it makes sense to me. As an alternative, i think 
moving the `emplace_back` outside of `serializeAPIRecord` is actually my other 
preferred arrangement; without some other way of calculating the path 
components on-the-fly (e.g. walking up DeclContexts) simply having 
`serializeAPIRecord` look at what's there and have the wrapper calls deal with 
setting up its state sounds the most reasonable to me. That way, as @zixuw 
said, the `emplace_back` and `pop_back` calls are at least lexically close to 
each other.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D123045: [clang][extract-api] Fix small issues with SymbolGraphSerializer

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added a comment.

In D123045#3427992 , @zixuw wrote:

> In D123045#3427699 , 
> @QuietMisdreavus wrote:
>
>> After a quick scan comparing the current output of these symbol graphs with 
>> the primary library used for reading them 
>> , the last thing i can spot 
>> that's "off" is that the "function signature" is currently being serialized 
>> under a `parameters` field instead of the required `functionSignature`.
>
> Hmm, I was looking at the format specification at 
> https://github.com/apple/swift-docc-symbolkit/blob/0a45209833f4a151212c1aa38e13cfc03b9462e4/openapi.yaml#L307,
>  and I searched the term `functionSignature` in the spec but found no 
> property with that name (except for the `FunctionSignature` schema that the 
> `parameters` property is referring to). But anyway this should be a easy fix.

It seems like the specification and implementation have diverged. The parser in 
swift-docc-symbolkit is looking for a `functionSignature` field by virtue of 
how it names its "coding key" 
.
 By comparison, here is the functionSignature emitter in the Swift symbol-graph 
generator 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123045

STAMPS
actor(@QuietMisdreavus) application(Differential) author(@dang) herald(H423) 
herald(H576) herald(H864) herald(H875) herald(H876) mention(@QuietMisdreavus) 
mention(@zixuw) monogram(D123045) object-type(DREV) 
phid(PHID-DREV-d5goxxqici5xa3w2bgjy) reviewer(@QuietMisdreavus) 
reviewer(@ributzka) reviewer(@zixuw) revision-repository(rG) 
revision-status(needs-review) subscriber(@cfe-commits) tag(#all) tag(#clang) 
via(web)

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123045: [clang][extract-api] Fix small issues with SymbolGraphSerializer

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added a comment.

After a quick scan comparing the current output of these symbol graphs with the 
primary library used for reading them 
, the last thing i can spot 
that's "off" is that the "function signature" is currently being serialized 
under a `parameters` field instead of the required `functionSignature`.




Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:510
   Symbols.emplace_back(std::move(*Obj));
+  PathComponentContext.pop_back();
 }

zixuw wrote:
> What's the cost/would it worth it to wrap the `emplace_back`s and `pop_back`s 
> of `PathComponentContext` in meaningful APIs like `enterPathComponentContext` 
> and `exitPathComponentContext`?
> That way the code is more self-explanatory and easier to read. It took me 
> some time and mental resources to figure out why the `pop_back` is placed 
> here.
What's the use of having the `emplace_back` call inside `serializeAPIRecord` 
but to pop it outside? It seems like it's easier to mess up for new kinds of 
records.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123045

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117809: [clang] Add an extract-api driver option

[Victoria Mitchell via Phabricator via cfe-commits]

QuietMisdreavus added inline comments.



Comment at: clang/include/clang/Driver/Types.def:103
 TYPE("hip-fatbin",   HIP_FATBIN,   INVALID, "hipfb",  
phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("api-information",  API_INFO, INVALID, "json",   
phases::Compile)
 TYPE("none", Nothing,  INVALID, nullptr,  
phases::Compile, phases::Backend, phases::Assemble, phases::Link)

Symbol graph files generally have the extension `.symbols.json` - is that 
something that should be reflected here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits