ioeric added inline comments.
================
Comment at: clang-tools-extra/clang-doc/Representation.cpp:49
+template <typename T>
+int isChildMergeNecessary(std::vector<T> &Children, T &ChildToMerge) {
+ for (unsigned long I = 0; I < Children.size(); I++) {
----------------
The function name doesn't describe what it does. Maybe `getChildIndexIfExists`?
================
Comment at: clang-tools-extra/clang-doc/Representation.h:190
+
+ std::vector<Reference> ChildNamespaces;
+ std::vector<Reference> ChildRecords;
----------------
Why are namespaces and records `Reference`s while others are actual infos.
Might worth a comment.
================
Comment at: clang-tools-extra/clang-doc/Representation.h:246
+
+ std::vector<Reference> ChildNamespaces;
+ std::vector<Reference> ChildRecords;
----------------
Would a record ever have namespace children? Maybe we should assert that this
doesn't happen?
================
Comment at: clang-tools-extra/clang-doc/Serialize.cpp:330
+ return nullptr;
+ std::unique_ptr<Info> IPtr = llvm::make_unique<NamespaceInfo>();
+ NamespaceInfo *I = static_cast<NamespaceInfo *>(IPtr.get());
----------------
`auto IPtr = llvm::make_unique<NamespaceInfo>();` should work well I think?
Converting the pointer back and forth seems unnecessary.
================
Comment at: clang-tools-extra/test/clang-doc/bc-linkage.cpp:106
+// CHECK-0-NEXT: <RecordBlock NumWords=107 BlockCodeSize=4>
+// CHECK-0-NEXT: <USR abbrevid=4 op0=20 op1=201 op2=179 op3=183 op4=26
op5=205 op6=216 op7=76 op8=91 op9=179 op10=32 op11=211 op12=78 op13=151
op14=103 op15=119 op16=21 op17=205 op18=179 op19=234 op20=50/>
+// CHECK-0-NEXT: <Name abbrevid=5 op0=10/> blob data = 'InnerClass'
----------------
I'm still a bit concerned about hardcoding a lot of USRs in tests. They are not
interpretable and generally not interesting for testing. Also as they are
auto-generated, it's hard to tell whether they are actually the desired USRs.
I'm concerned because the maintenance is getting higher as number of tests
grows - everyone changing USR semantics in the future has to know to regenerate
clang-doc tests, this can be annoying and potentially unmanageable when a small
change in clang USR requires changes to many test files in clang-tools-extra :(
Comparing to the value it brings to test USRs in all tests, I'd still suggest
simply matching them with a `{{.*}}`and only test USRs in few tests where you
are actually interested in them.
https://reviews.llvm.org/D48341
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
