This is an archive of the discontinued LLVM Phabricator instance.

[clang][extract-api] Add support for macros
ClosedPublic

Authored by dang on Mar 28 2022, 1:50 PM.

Details

Summary

To achieve this we hook into the preprocessor during the
ExtractAPIAction and record definitions for macros that don't get
undefined during preprocessing.

Diff Detail

Event Timeline

dang created this revision.Mar 28 2022, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 1:50 PM
dang requested review of this revision.Mar 28 2022, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 1:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zixuw added a comment.Mar 28 2022, 2:15 PM

Still going through the declaration fragments builder and ExtractAPIVisitor/Consumer/Action changes, but it seems that the patch needs a rebase onto the latest main right now as it's missing several already landed changes.

clang/include/clang/ExtractAPI/API.h
127

Now that we are re-ordering it, would it look more organized if we push GVKind Kind further down so that the two StringRefs could be adjacent, and we also keep a common list of arguments (Name, USR, Loc, Availability, ..., SubHeading) the same across all kinds of records upfront? So maybe push to the end after SubHeading and before Signature.

127

Also it seems that it becomes a requirement for the *Record c'tors that Name should be the first parameter in order to use the addTopLevelRecord template, though that's hidden in an anonymous namespace in the implementation file.
How can we communicate or document this clearly for future extensions?

385

Don't we also need sub-headings for macros?

391

Need out-of-line virtual anchor as this struct inherits a virtual table.

zixuw added inline comments.Mar 28 2022, 5:31 PM
clang/include/clang/ExtractAPI/Serialization/SymbolGraphSerializer.h
22

Why do we need this include?

clang/lib/ExtractAPI/API.cpp
31

Does it make sense to just directly make the *RecordMap types in APISet as a template using?

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
634–645

Worth moving this change and the APISet move and relevant structural changes into its own patch, as mentioned in D122446.

clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp
17

Why do we need this include?

dang marked 7 inline comments as done.Mar 29 2022, 9:47 AM
dang added inline comments.
clang/include/clang/ExtractAPI/API.h
127

Good catch! Correct, it isn't really a requirement in the sense that we could pass it in twice but since this was the only place that needed the change I felt it would improve the ergonomics of addTopLevelRecord to make the change here.

The way I see it it depends on whether we want to make use of addTopLevelRecord to associate stuff in the relevant map using a key that isn't the name of the record. If we don't want to do that, it might make sense to make the maps sets, which would convey the intent a bit better IMHO, but that would be a follow up patch.

Alternatively if we want to keep the structure as is, the easiest thing to do is to put a comment on top of the APIRecord base class that says that derived classes need to put the Name parameter as the first thing.

clang/lib/ExtractAPI/API.cpp
31

Yup agreed!

dang marked an inline comment as done.Mar 29 2022, 10:13 AM
dang added inline comments.
clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
634–645
dang updated this revision to Diff 418948.Mar 29 2022, 12:05 PM

Address review feedback.

zixuw added inline comments.Mar 29 2022, 3:11 PM
clang/include/clang/ExtractAPI/API.h
559

Extra/should-be-empty line here?

clang/lib/ExtractAPI/API.cpp
19

Do we need to include declaration fragments from API.cpp?

31

Now can we use APISet::RecordMap<RecordTy> as the type of the first param here?
Can also get rid of including MapVector.h

dang updated this revision to Diff 419207.Mar 30 2022, 10:20 AM
dang marked 3 inline comments as done.

Update with code review feedback and the rebased code.

zixuw accepted this revision.Mar 30 2022, 10:26 AM

LGTM!

This revision is now accepted and ready to land.Mar 30 2022, 10:26 AM
dyung added a subscriber: dyung.Mar 30 2022, 11:28 AM

This change is causing 9 test failures on a buildbot, can you please take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/139/builds/19441

thakis added a subscriber: thakis.Mar 30 2022, 12:20 PM

Looks like either this or your other commit at the same time broke tests on windows: http://45.33.8.238/win/55345/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

(Also, please add a "Differential Revision: https://reviews.llvm.org/D122611" line to the bottom of your commit, to make it easy to find the review from the commit message.)

dang added a comment.Mar 30 2022, 12:33 PM

I have what should be a NFC fix that should fix the issue, just checking everything still works and will commit it.

dang added a comment.Mar 30 2022, 12:41 PM

Committed a revert of the changes in that particular function in commit 985eaa1a3da2a1b88ea70a65ffd5783aa82ea65e

dang added a comment.Mar 30 2022, 12:56 PM

@thakis I am UK based so I am logging off as it's 9pm over here. The fix I committed has fixed the issue on other buildbots that were failing due to this. If it still fails on Windows, would you be able to revert the change for me?

My windows bot cycled green, thanks!