<div class="gmail_quote">On Mon, Jun 25, 2012 at 9:34 AM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
Attached is the first of seven patches for the -verify feature, split up as<br>
requested!<br>
<br>
The division is as follows:<br>
<br>
Part 1: preparation; make ExpectedData a member variable to<br>
VerifyDiagnosticConsumer instance and move Directive<br>
and ExpectedData classes into header.</blockquote><div><br></div><div>Is there a reason to make these member classes public? Seems fine otherwise. This is needed by part 5, right?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
Part 2: Implement alternative line number syntax<br></div></blockquote><div><br></div><div>+ // next optional token: @</div><div><br></div><div>Comments should start with a capital letter (and usually end with a full stop).</div>
<div><br></div><div>+ SourceLocation RealPos = Pos;</div><div><br></div><div>This name could be clearer. Maybe ExpectedLoc, to indicate this is the location where we expect the diagnostic? Where does the 'verify failed' diagnostic point in the case when the expectation isn't met? Perhaps we should maintain two SourceLocations in a Directive, to indicate where the directive was written and where the diagnostic is expected, so we can point at the directive itself in that case?</div>
<div><br></div><div>+ if (PH.Next("-")) { // previous to current line</div><div><br></div><div>Please put the comment on a line of its own.</div><div><br></div><div>+ } else if (PH.Next("+")) { // subsequent to current line</div>
<div><br></div><div>I'd prefer that the "-" and "+" cases be merged together, since their implementations are nearly identical.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
</div>Part 3: Implement ranges for diagnostic appearances</blockquote><div><br></div><div>+ // a positive integer can be followed by a '+'</div><div><br></div><div>As above, please start this comment with a capital letter.</div>
<div><br></div><div>+ if (PH.Next(Max)) PH.Advance();</div><div>+ if (Max < Min) Max = Min;</div><div><br></div><div>Maybe we should issue a diagnostic in the 'else' cases here?</div><div><br></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
Part 4: Fix the swallowing of verify diagnostics following<br>
diagnostics causing suppression - e.g. fatal error<br></div></blockquote><div><br></div><div><div>@@ -634,9 +634,10 @@</div><div> </div><div> // If the client doesn't care about this message, don't issue it. If this is</div>
<div> // a note and the last real diagnostic was ignored, ignore it too.</div><div>- if (DiagLevel == DiagnosticIDs::Ignored ||</div><div>- (DiagLevel == DiagnosticIDs::Note &&</div><div>- Diag.LastDiagLevel == DiagnosticIDs::Ignored))</div>
<div>+ if (!Diag.ForceEmitDiagnostics &&</div><div>+ (DiagLevel == DiagnosticIDs::Ignored ||</div><div>+ (DiagLevel == DiagnosticIDs::Note &&</div><div>+ Diag.LastDiagLevel == DiagnosticIDs::Ignored)))</div>
<div> return false;</div></div><div><br></div><div>I don't think we want this change; I think we should only be overriding diagnostic limits, not -Wno-foo. In particular, we have code which checks whether a diagnostic is enabled before computing whether to emit the diagnostic, and this change makes that not be a pure optimization any more. In any case, we still want to suppress notes if the preceding diagnostic was suppressed.</div>
<div><br></div><div>The rest of this part looks fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
</div>Part 5: Implement conditional diagnostic checking, moving<br>
from post-processing of source file to using the<br>
CommentHandler interface<br></blockquote><div><br></div><div>The names Seen and Unseen are quite opaque to me. FilesWithDiagnostics and FilesWithDirectives would be much clearer. I also don't yet understand why we would get into the situation where we need the special-case code to scan the FilesWithDiagnostics which we didn't find directives in. Is this related to modules / PCH / PPH? Can you point out an example test where this comes up?</div>
<div><br></div><div>I also don't like the coupling between DiagnosticConsumer and CommentHandler which the getCommentHandler method introduces. This seems to exist only to support part 6, so I wonder if there's a better way to approach that.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Part 6: Update CaptureDiagnosticConsumer in ARCMT.cpp to<br>
also capture and forward the comment lines, as<br>
well as the diagnostics<br></blockquote><div><br></div><div>We need this to fix ARCMT tests broken by part 5, right? I don't know anything about this code, so hopefully someone else will look over these changes.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Part 7: Test cases</blockquote><div><br></div><div> These should be split up to go with the related code changes above.</div></div>