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 

```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 

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).


cfe-commits mailing list

Reply via email to