<div dir="ltr">Looks good, please commit!<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Dec 27, 2013 at 10:39 AM, Will Wilson <span dir="ltr"><<a href="mailto:will@indefiant.com" target="_blank">will@indefiant.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi All,</div><br><div class="gmail_extra">Thanks for the great feedback. I've fixed the comment formatting issue, the spurious typedef for the iterator and integrated Reid's extended test proposal.<br>

<div class="gmail_quote"><br></div><div class="gmail_quote">The other points:</div><div class="im"><div class="gmail_quote"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<span style="font-family:arial,sans-serif;font-size:13px">+    if (!FileEnt)<br></span><span style="font-family:arial,sans-serif;font-size:13px">+      FileEnt = SourceMgr.getFileEntryForID(</span><span style="font-family:arial,sans-serif;font-size:13px">SourceMgr.getMainFileID());<br>

</span><span style="font-family:arial,sans-serif;font-size:13px">+<br></span><span style="font-family:arial,sans-serif;font-size:13px">+    if (FileEnt)<br></span><span style="font-family:arial,sans-serif;font-size:13px">+      Includers.push_back(FileEnt);</span><br style="font-family:arial,sans-serif;font-size:13px">

<span style="font-family:arial,sans-serif;font-size:13px">Is the second if statement needed?  Could we assert(FileEnt); here?</span></blockquote><div class="gmail_quote"><br></div></div><div class="gmail_quote">FileEnt is NULL in a few test cases so I've kept the conditional for now. I didn't get chance to investigate further but the behavior should be unchanged from the previous revision.</div>
<div class="im">
<div class="gmail_quote"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-family:arial,sans-serif;font-size:13px">Will, do you think it's worth adding a diagnostic (warning) for this<br>

</span><span style="font-family:arial,sans-serif;font-size:13px">new behavior, in the case where the new/MSVC behavior differs from the<br></span><span style="font-family:arial,sans-serif;font-size:13px">old/GCC behavior? Something like</span><br style="font-family:arial,sans-serif;font-size:13px">

<span style="font-family:arial,sans-serif;font-size:13px">    warning: in Microsoft mode this #include directive refers to<br></span><span style="font-family:arial,sans-serif;font-size:13px">'a/b/foo.h', where<br>

</span><span style="font-family:arial,sans-serif;font-size:13px">    normal header lookup would have found 'c/foo.h' instead [-Wmsvc-include]</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">Silently accepting the construct just encourages people to rely on it.<br>

</span><span style="font-family:arial,sans-serif;font-size:13px">Giving a diagnostic would at least let the relevant developers know<br></span><span style="font-family:arial,sans-serif;font-size:13px">that something bogus is going on and that maybe they should fix their<br>

</span><span style="font-family:arial,sans-serif;font-size:13px">directory structure. (And if they really don't care, they can silence<br></span><span style="font-family:arial,sans-serif;font-size:13px">the warning.)</span></blockquote>

<div class="gmail_quote"><br></div></div><div class="gmail_quote">An excellent idea. I've implemented what I hope is a reasonable middle ground between performance and diagnostics. Running the normal lookup rules just for a (probably ignored) warning seemed a little heavy handed. So in this case I warn with:</div>

<div class="gmail_quote"><br></div></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div class="gmail_extra"><div class="gmail_quote">warning: #include resolved using non-portable MSVC search rules as: path/to/foo.h [-Wmsvc-include]</div>

</div></blockquote><div><br></div><div>Let me know what you think and I'll get it in if everyone's happy with it.</div><div><br></div><div>Cheers,</div><div>Will.</div></div>
</blockquote></div><br></div>