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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
