[PATCH] D134371: [clang-doc] Add typedef/using information.
Paul Kirth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 22 14:30:33 PDT 2022
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
More information about the cfe-commits
mailing list