dang requested changes to this revision.
dang added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:102
- // Add a new Fragment to the beginning of the Fragments.
- DeclarationFragments &appendFront(StringRef Spelling, FragmentKind Kind,
- StringRef PreciseIdentifier = "",
- const Decl *Declaration = nullptr) {
- Fragments.emplace(Fragments.begin(), Spelling, Kind, PreciseIdentifier,
- Declaration);
+ size_t calculateOffset(intmax_t Index) const {
+ if (Index >= 0) {
----------------
This shouldn't need to be part of the public interface of the
DeclarationFragments type and can just be a static function in
DeclarationFragments.cpp.
================
Comment at: clang/include/clang/ExtractAPI/DeclarationFragments.h:114
+ // Add a new Fragment at an arbitrary offset.
+ DeclarationFragments &insertAtIndex(intmax_t Index, StringRef Spelling,
+ FragmentKind Kind,
----------------
Is this parameter `intmax_t` to allow negative indices? If so I really don't
think it's a good idea. I would prefer if we exposed an iterator interface to
DeclarationFragments to make the API more like that of a vector type. (The
iterator would just need to be the iterator for the underlying vector).
================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:119
+ .insertAtIndex(1, " ", DeclarationFragments::FragmentKind::Text)
+ .insertAtIndex(-1, " { ... } ",
+ DeclarationFragments::FragmentKind::Text)
----------------
I don't think the API naming makes it clear that you can and what it means to
insert at a negative offset. I would much rather we had an iterator based
interface and didn't necessarily do chained calls. It could look something like
this:
```
auto &DeclFrag = Record.second->Declaration;
DeclFrag.insert(DeclFrag.begin(), "typedef",
DeclarationGragments::FragmentKind::Keyword, "", nullptr);
// the rest of the code
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151048/new/
https://reviews.llvm.org/D151048
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
