[PATCH] D134371: [clang-doc] Add typedef/using information.

[Brett Wilson via Phabricator via cfe-commits]

brettw added inline comments.


================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:540
 }
-
 template <> void addChild(NamespaceInfo *I, EnumInfo &&R) {
----------------
paulkirth wrote:
> nit: can we avoid unrelated changes to  whitespace here and elsewhere in the patch?
I did this on purpose. There were 8 variants of AddChild and I added two more. It became difficult to find the right one so I grouped them with comments for each group. This may be difficult to see in the side-by-side but it's much nicer in an editor.


================
Comment at: clang-tools-extra/clang-doc/Representation.h:280
 // Info for namespaces.
-struct NamespaceInfo : public Info {
+struct NamespaceInfo : public Info, public ScopeHasChildren {
   NamespaceInfo(SymbolID USR = SymbolID(), StringRef Name = StringRef(),
----------------
paulkirth wrote:
> It seems a bit odd to use inheritance here on a type w/ public fields, and no methods. do you think using composition would improve the situation?
> 
>  I'm fine w/ it if we think this is the simpler/more maintainable solution, but given that we don't have any methods in this case I'm unsure if there's much benefit.
I don't have a strong opinion. I changed to composition.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:633
+  // both the parent and the record itself.
+  return {std::move(I), MakeAndInsertIntoParent<const RecordInfo &>(*I)};
 }
----------------
paulkirth wrote:
> Won't this deref the moved from `unique_ptr`? 
> 
> https://en.cppreference.com/w/cpp/language/eval_order
> > In list-initialization, every value computation and side effect of a given initializer clause is sequenced before every value computation and side effect associated with any initializer clause that follows it in the brace-enclosed comma-separated list of initializers.
> 
> Under that reading, the move //should// always happen first. If it happens to still work, I think it's just luck in the implementation.
Thanks, nice catch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134371/new/

https://reviews.llvm.org/D134371