[PATCH] D151048: [clang][ExtractAPI] Modify declaration fragment methods to add a new fragment at an arbitrary offset.

[Daniel Grumberg via Phabricator via cfe-commits]

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