[PATCH] D41102: Setup clang-doc frontend framework

[Jonas Toth via Phabricator via cfe-commits]

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