[PATCH] D122611: [clang][extract-api] Add support for macros

[Daniel Grumberg via Phabricator via cfe-commits]

dang marked 7 inline comments as done.
dang added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/API.h:117
 
-  GlobalRecord(GVKind Kind, StringRef Name, StringRef USR, PresumedLoc Loc,
+  GlobalRecord(StringRef Name, GVKind Kind, StringRef USR, PresumedLoc Loc,
                const AvailabilityInfo &Availability, LinkageInfo Linkage,
----------------
zixuw wrote:
> zixuw wrote:
> > Now that we are re-ordering it, would it look more organized if we push `GVKind Kind` further down so that the two `StringRef`s 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`.
> 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?
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.


================
Comment at: clang/lib/ExtractAPI/API.cpp:32
+RecordTy *
+addTopLevelRecord(MapVector<StringRef, std::unique_ptr<RecordTy>> &RecordMap,
+                  StringRef Name, CtorArgsTy &&...CtorArgs) {
----------------
zixuw wrote:
> Does it make sense to just directly make the `*RecordMap` types in `APISet` as a `template using`?
Yup agreed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122611