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