JonasToth added inline comments.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:157
+ if (C->getNumAttrs() > 0) {
+ for (unsigned i = 0, e = C->getNumAttrs(); i != e; ++i) {
+ const HTMLStartTagComment::Attribute &Attr = C->getAttr(i);
----------------
minor nit: the loop condition is correct! when reading really fast one might
overlook the if above and wonder if `i < e` might be better. but this is
opinionated and just a suggestion.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:171
+ CurrentCI->Name = getCommandName(C->getCommandID());
+ for (unsigned i = 0, e = C->getNumArgs(); i > e; ++i)
+ CurrentCI->Args.push_back(C->getArgText(i));
----------------
Now I have a question :)
the condition `i > e` seems odd to me. `i == 0` in the first iteration and i
expect `e > 0` so this loop should never execute or did I oversee something?
Same below
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:228
+ for (comments::Comment *Child :
+ llvm::make_range(C->child_begin(), C->child_end())) {
+ CommentInfo ChildCI;
----------------
juliehockett wrote:
> JonasToth wrote:
> > Extract range into utility method of `Comment`
> See above -- `comments::Comment` is a clang type that stores all the
> information about a particular piece of a comment -- the `CommentInfo` struct
> is specific to the clang-doc setup. Is that what you're thinking about?
I was just thinking that creating this range here is clumsy. But if there is no
way to fix that easily you can leave it as is.
This means the suggested `Children` thing above is a non-issue is it? If yes
just ignore my comments then :)
================
Comment at: tools/clang-doc/ClangDocReporter.h:51
+ llvm::SmallVector<int, 8> Position;
+ std::vector<CommentInfo> Children;
+};
----------------
juliehockett wrote:
> JonasToth wrote:
> > Here a short `children()` method return llvm::make_range shortens the code
> > in a later loop and might benefit in the future for iterations over
> > children.
> Is there a reason you wouldn't be able to just use `for (const CommentInfo &c
> : CI.Children)` ? The later loop I believe you're referencing doesn't loop
> over this struct, it looks at the children of a `comments::Comment` type.
Yes of course. But you can not enforce that the user of `Children` will not
modify it. This is a struct with seemingly no constraints on its values meaning
that would be ok.
If some values belong together and must work together and kept consistent and
`const` method returning `const&` to `Children` might be better. Modfying
`Children` can then be done via methods that ensure consistency between the
values.
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
