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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett marked 2 inline comments as done.
juliehockett added inline comments.


================
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:
> juliehockett wrote:
> > 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?
> Thanks! 
> 
> FWIW, I wouldn't check the length either as it seems to add too much overhead; I think checking length/USR in one test should get it well covered. 
Possibly for the YAML tests, but for the bitcode ones I also want to check the ops, so it's not just the USR text. It's also autogenerated, so the overhead is minimal. 

Also, just to reiterate, you'll still likely have to regenerate these if you dramatically change the USR spec. The bitcode tests all rely on USRs to write to files, and if those USRs change the name of the file the test needs to read will also change.


https://reviews.llvm.org/D48341