The tests in llvm just get the whole flags enum and then format it. So if this flag is set in the output, you’ll see it. But it doesn’t seem easy to make that formatting function go through this method. That said, I think the important thing to test is that this flag properly gets set in the enum in the correct condition. So the current test tests that I think. But this helper method seems obviously correct (and pretty trivial) and not worth adding a special test for<br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 11:36 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Maybe we do have coverage but not through this member function - if the code for nested classes that is exercised by LLVM lit or unit tests (assuming it's tested somewhere) could be changed to use this member function, then it'd be tested & I'd be good with it.<br><br>Yeah, I don't mind what form the tests take - but just asking the general "this is a change to LLVM, does it/can it/should it have a test in LLVM?"</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 11:10 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I believe we do already have test coverage for nested classes, i was just adding this helper method so I didn’t have to write the long if statement. But if we don’t have coverage for nested types, then maybe a lit test would be better?<br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 11:04 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">This is, 'technically', a separate patch in some sense from the LLD part - changes to LLVM usually merit tests in LLVM to ensure they work/don't regress/etc without needing to checkout & test other subprojects (like lld).</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 11:02 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This one specific function, or the patch as a whole? I thought the patch had pretty good non-unit-test coverage <br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 10:36 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Could this use/be tested with a unit test, perhaps?</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Nov 8, 2018 at 10:52 AM Zachary Turner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: zturner<br>
Date: Thu Nov 8 10:50:11 2018<br>
New Revision: 346429<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=346429&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=346429&view=rev</a><br>
Log:<br>
[NativePDB] Higher fidelity reconstruction of AST from Debug Info.<br>
<br>
In order to accurately put a type into the correct location in the AST<br>
we construct from debug info, we need to be able to determine what<br>
DeclContext (namespace, global, nested class, etc) that it goes into.<br>
PDB doesn't contain this mapping. It does, however, contain the reverse<br>
mapping. That is, for a given class type T, you can determine all<br>
classes Q1, Q2, ..., Qn that are nested inside of T. We need to know,<br>
for a given class type Q, what type T is it nested inside of.<br>
<br>
This patch builds this map as a pre-processing step when we first<br>
load the PDB by scanning every type. Initial tests show that while<br>
this can be slow in debug builds of LLDB, it is quite fast in release<br>
builds (less than 2 seconds for a ~1GB PDB, and it only needs to happen<br>
once).<br>
<br>
Furthermore, having this pre-processing step in place allows us to<br>
repurpose it for building up other kinds of indexing to it down the<br>
line. For the time being, this gives us very accurate reconstruction<br>
of the DeclContext hierarchy.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D54216" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54216</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h<br>
<br>
Modified: llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h?rev=346429&r1=346428&r2=346429&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h?rev=346429&r1=346428&r2=346429&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h (original)<br>
+++ llvm/trunk/include/llvm/DebugInfo/CodeView/TypeRecord.h Thu Nov 8 10:50:11 2018<br>
@@ -429,6 +429,10 @@ public:<br>
return (Options & ClassOptions::ForwardReference) != ClassOptions::None;<br>
}<br>
<br>
+ bool containsNestedClass() const {<br>
+ return (Options & ClassOptions::ContainsNestedClass) != ClassOptions::None;<br>
+ }<br>
+<br>
bool isScoped() const {<br>
return (Options & ClassOptions::Scoped) != ClassOptions::None;<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>