[PATCH] D48341: [clang-doc] Refactoring mapper to map by scope

Julie Hockett via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 31 17:29:52 PDT 2018


juliehockett added inline comments.


================
Comment at: clang-tools-extra/clang-doc/Representation.h:246
+
+  std::vector<Reference> ChildNamespaces;
+  std::vector<Reference> ChildRecords;
----------------
ioeric wrote:
> Would a record ever have namespace children? Maybe we should assert that this doesn't happen?
You're right, that's actually not valid in C++. Removed the field.


================
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'
----------------
ioeric wrote:
> 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.
Okay, I updated it to only check the length -- is that reasonable?


https://reviews.llvm.org/D48341





More information about the cfe-commits mailing list