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