[PATCH] D41102: Setup clang-doc frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-doc/ClangDoc.cpp:32
+  ECtx.reportResult(
+      Name, Mapper.emitInfo(D, getComment(D), Name, getLine(D), getFile(D)));
+}
----------------
lebedev.ri wrote:
> I wonder if `Name` should be `std::move()`'d ? Or not, `reportResult()` seems to take `StringRef`...
> 
> (in general, it might be a good idea to run clang-tidy on the code)
So the `ExecutionContext` can do implement different ways to do this -- in this case, the default container created is the `InMemoryToolResults`, which technically takes in `StringRef`s, but copies their data to its in-memory representation: 

```void InMemoryToolResults::addResult(StringRef Key, StringRef Value) {
    KVResults.push_back({Key.str(), Value.str()});
}```

A different implementation of it (i.e. a results container not in memory) would likely have to be backed by a file, so the data would be written out there anyways.


================
Comment at: clang-doc/ClangDocBinary.cpp:72
+  assert(Abbrevs.find(recordID) == Abbrevs.end() &&
+         "Abbreviation already set.");
+  Abbrevs[recordID] = abbrevID;
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > So it does not *set* the abbreviation, since it is not supposed to be called if the abbreviation is already set, but it *adds* a unique abbreviation.
> > I think it should be called `void AbbreviationMap::add(unsigned recordID, unsigned abbrevID)` then
> This is marked as done, but the name is still the same, and no counter-comment was added, as far as i can see
It was changed to AbbreviationMap::add from AbbreviationMap::set, as you suggested -- unless I missed something in your comment?


================
Comment at: clang-doc/ClangDocBinary.h:82
+
+static std::map<BlockId, std::string> BlockIdNameMap = {
+  {NAMESPACE_BLOCK_ID, "NamespaceBlock"},
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > Nice!
> > Some thoughts:
> > 1. I agree it makes sense to keep it close to the enum definition, in header...
> > 2. This will result in global constructor. Generally they are frowned upon in LLVM. But since this is a standalone binary, it may be ok?
> > 3. Have you tried using `StringRef` here, instead of `std::string`?
> > 4. `std::map` is in general a bad idea.
> >       Since the `enum`'s enumerators are all small and consecutive, maybe try `llvm::IndexedMap`?
> Also, this should be `static const`, since the underlying enum won't change on the fly.
> 
> `#llvm` suggests to use TableGen here, i'm not sure how that would work.
> 
> As i have now noticed, there isn't a init-list constructor, so I think **something** like this might work:
> ```
> static const llvm::IndexedMap<BlockId> BlockIdNameMap = []() {
>   llvm::IndexedMap<BlockId> map;
>   map.reserve(BI_LAST);
> 
>   // There is no init-list constructor for the IndexedMap, so have to improvise
>   static const std::initializer_list<std::pair<BlockId, const char* const>> inits = {
>     {NAMESPACE_BLOCK_ID, "NamespaceBlock"},
>     ...
>   };
>   for(const auto& init : inits)
>     map[init.first] = init.second;
> }();
> ```
> 
> Also, even though `llvm::IndexedMap<>` is using `llvm::SmallVector<>` internally, it does not expose the initial size as template parameter, unfortunately, but hardcodes it to `0`. I think it would be great to add one more template parameter to `llvm::IndexedMap<>`, which would default to `0`, but would allow us here to avoid all memory allocation altogether.
> 
> What do you think? If you do agree that using `IndexedMap` seems like the right choice, but **don't** want to write the patch for template parameter, i might look into it..
Had to play with it a bit, but it's working now.

For the template parameter, I'm happy to take a look! Avoiding allocation here would be great.


================
Comment at: clang-doc/ClangDocMapper.cpp:202
+  for (comments::Comment *Child :
+       make_range(C->child_begin(), C->child_end())) {
+    CurrentCI.Children.emplace_back(std::make_shared<CommentInfo>());
----------------
lebedev.ri wrote:
> It would be nice if you could (as a new Differential) add a `children()` function to that class that will do that automatically.
Will do :)  (and same for the below)


================
Comment at: clang-doc/ClangDocMapper.h:36
+
+class ClangDocCommentVisitor
+    : public ConstCommentVisitor<ClangDocCommentVisitor> {
----------------
sammccall wrote:
> why is this exposed?
> (and what does it do?)
Moved it into the mapper class, but it traverses a comment and extracts its information into the `CommentInfo` struct


================
Comment at: clang-doc/ClangDocMapper.h:66
+  template <class C>
+  StringRef emitInfo(const C *D, const FullComment *FC, StringRef Key,
+                     int LineNumber, StringRef File);
----------------
sammccall wrote:
> when returning a stringref, it might pay to be explicit about who owns the data, so the caller knows the safe lifetime.
> (This isn't always spelled out in llvm, but should probably be done more often!)
Definitely reasonable, particularly since this one was left over from a past diff where the string buffer was created externally, and so now it shouldn't be returning the ref (as you noticed below).


https://reviews.llvm.org/D41102