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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

Can we also add support for some of these changes to the other backends? I'm fine w/ us delaying that a bit, and tackling those in separate patches, but we shouldn't let one backend get wildly ahead of the others.



================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:540
 }
-
 template <> void addChild(NamespaceInfo *I, EnumInfo &&R) {
----------------
nit: can we avoid unrelated changes to  whitespace here and elsewhere in the patch?


================
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(),
----------------
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.


================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:450
+  const DeclContext *DC = D->getDeclContext();
+  do {
     if (const auto *N = dyn_cast<NamespaceDecl>(DC)) {
----------------
Is this guaranteed to always execute at least once? can't `getDeclContext()` return null?

If there's some invariant that `D` is non null, then this code should probably have an assert.


================
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)};
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134371