This is an archive of the discontinued LLVM Phabricator instance.

[clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef
ClosedPublic

Authored by Ruturaj4 on Mar 19 2023, 10:52 AM.

Details

Summary

enums and structs declared inside typedefs have incorrect declaration fragments, where the typedef keyword and other syntax is missing.

For the following struct:

typedef struct Test {

int hello;

} Test;
The produced declaration is:

"declarationFragments": [

{
  "kind": "keyword",
  "spelling": "struct"
},
{
  "kind": "text",
  "spelling": " "
},
{
  "kind": "identifier",
  "spelling": "Test"
}

],
instead the declaration fragments should represent the following

typedef struct Test {

} Test;

This patch removes the condition in SymbolGraphSerializer.cpp file and completes declaration fragments

Diff Detail

Event Timeline

Ruturaj4 created this revision.Mar 19 2023, 10:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
Ruturaj4 requested review of this revision.Mar 19 2023, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 10:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dang requested changes to this revision.Mar 20 2023, 7:05 AM

I think there might be some code missing here. Also can you add a test?

This revision now requires changes to proceed.Mar 20 2023, 7:05 AM
Ruturaj4 updated this revision to Diff 507958.Mar 23 2023, 9:53 PM
  1. Updating D146385: [clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Updated previous diff with requested changes and added new test.

dang added inline comments.Mar 24 2023, 3:38 AM
clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
342

This doesn't quite produce the output we want, this would generate typedef struct Foo; instead of the expected typedef struct Foo { ... } Bar; where Foo is the name of the struct and Bar is the name of the typedef. This should be easy enough to fix.

347

What happens if the visitation hasn't already encountered the definition of the struct? We wouldn't find it in the the APISet and this doesn't work. The approach I would suggest here is to generate the fragments for the underlying Decl directly. For example what happens for the following code:

c
struct Foo;
typedef struct Foo TypedefedFoo;
struct Foo {
    int bar;
};
349–354

This logic is very similar to the one below we could use the same code.

Ruturaj4 updated this revision to Diff 508375.Mar 25 2023, 10:07 PM
  1. Updating D146385: [clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Made all updates as you requested, though I didn't need to generate fragments for the underlying Decl. I added an example as you have recommended.

Ruturaj4 marked 2 inline comments as done.Mar 25 2023, 10:10 PM
Ruturaj4 added inline comments.
clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
347

Actually added an example for this. Our code works fine. Please take a look.

Ruturaj4 updated this revision to Diff 509004.Mar 28 2023, 7:11 AM
  1. Updating D146385: [clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Update main to avoid build failures.

Ruturaj4 marked an inline comment as done.Mar 28 2023, 8:10 AM
dang added a comment.Mar 29 2023, 8:15 AM

You will need to rebase this as I made some changes recently to how ExtractAPIVisitor is structured. We can either set up a time to talk about it and do it together or I can handle doing this work once we are happy with this.

clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
52

this should be a marked static or put in an anonymous namespace.

56

I think this warrants adding methods to declaration fragments to streamline this sort of operation (merging fragments at arbitrary offsets. This should probably be done as a follow up patch.

Ruturaj4 updated this revision to Diff 509472.Mar 29 2023, 1:53 PM
  1. Updating D146385: [clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

declare modifyRecord function "static"

Ruturaj4 marked an inline comment as done.Mar 29 2023, 1:54 PM
Ruturaj4 added inline comments.
clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
56

Ok I will add this in a subsequent patch.

Ruturaj4 updated this revision to Diff 509473.Mar 29 2023, 1:56 PM
  1. Updating D146385: [clang][ExtractAPI] Complete declaration fragments for TagDecl types defined in a typedef #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

rebase and update patch.

dang accepted this revision.Mar 30 2023, 5:59 AM
This revision is now accepted and ready to land.Mar 30 2023, 5:59 AM
This revision was landed with ongoing or failed builds.Apr 13 2023, 9:56 AM
This revision was automatically updated to reflect the committed changes.