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