<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META content=text/html;charset=iso-8859-1 http-equiv=Content-Type>
<META name=GENERATOR content="MSHTML 8.00.7601.17824"></HEAD>
<BODY style="PADDING-LEFT: 10px; PADDING-RIGHT: 10px; PADDING-TOP: 15px"
id=MailContainerBody leftMargin=0 topMargin=0 CanvasTabStop="true"
name="Compose message area">
<DIV><FONT size=2 face=Consolas>On Thursday, June 28, 2012 2:12 AM, Richard
Smith wrote:<BR>> On Mon, Jun 25, 2012 at 9:34 AM, Andy Gibbs wrote:<BR>>
<BR>>> Hi,<BR>>> <BR>>> Attached is the first of seven patches
for the -verify feature, split up<BR>>> as
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.<BR>><BR>> Is there a reason to make
these member classes public? Seems fine otherwise.<BR>> This is needed by
part 5, right?</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Correct.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>>> Part 2: Implement alternative line
number syntax<BR>><BR>> + // next optional token:
@<BR>><BR>> Comments should start with a capital letter (and usually end
with a full<BR>> stop).</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>No problem; will do.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>> + SourceLocation RealPos
= Pos;<BR>><BR>> This name could be clearer. Maybe ExpectedLoc, to
indicate this is the<BR>> location where we expect the diagnostic? Where does
the 'verify failed'<BR>> diagnostic point in the case when the expectation
isn't met? Perhaps we <BR>> should maintain two SourceLocations in a
Directive, to indicate where the<BR>> directive was written and where the
diagnostic is expected, so we can point<BR>> at the directive itself in that
case?</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Oh, I thought RealPos was fairly clear, but
accept that your name is better!<BR>I agree also with the idea of keeping both
locations -- I will look into it.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>> + if
(PH.Next("-")) { // previous to current line<BR>><BR>> Please put the
comment on a line of its own.<BR>><BR>> + }
else if (PH.Next("+")) { // subsequent to current line<BR>><BR>> I'd
prefer that the "-" and "+" cases be merged together, since their<BR>>
implementations are nearly identical.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Ok, will do.<BR> <BR>>> Part 3:
Implement ranges for diagnostic appearances<BR>><BR>>
+ // a positive integer can be followed by a
'+'<BR>><BR>> As above, please start this comment with a capital
letter.<BR>> <BR>> + if
(PH.Next(Max)) PH.Advance();<BR>> +
if (Max < Min) Max = Min;<BR>><BR>> Maybe we should issue a diagnostic
in the 'else' cases here?</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Ok.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>>> Part 4: Fix the swallowing of verify
diagnostics
following<BR>>>
diagnostics causing suppression - e.g. fatal error<BR>><BR>> @@ -634,9
+634,10 @@<BR>> <BR>> // If the client doesn't care
about this message, don't issue it. If this is<BR>>
// a note and the last real diagnostic was ignored, ignore it too.<BR>>
- if (DiagLevel == DiagnosticIDs::Ignored ||<BR>>
- (DiagLevel == DiagnosticIDs::Note
&&<BR>> - Diag.LastDiagLevel ==
DiagnosticIDs::Ignored))<BR>> + if (!Diag.ForceEmitDiagnostics
&&<BR>> + (DiagLevel ==
DiagnosticIDs::Ignored ||<BR>>
+ (DiagLevel ==
DiagnosticIDs::Note &&<BR>>
+ Diag.LastDiagLevel
== DiagnosticIDs::Ignored)))<BR>> return
false;<BR>><BR>> I don't think we want this change; I think we should only
be overriding<BR>> diagnostic limits, not -Wno-foo. In particular, we have
code which checks<BR>> whether a diagnostic is enabled before computing
whether to emit the<BR>> diagnostic, and this change makes that not be a pure
optimization any more.<BR>> In any case, we still want to suppress notes if
the preceding diagnostic<BR>> was suppressed.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Hmm, this looks like it might be in by
mistake. My apologies; I have many</FONT></DIV>
<DIV><FONT size=2 face=Consolas>patches in by own branch and occasionally
they cross over, though I do double-</FONT></DIV>
<DIV><FONT size=2 face=Consolas>check to try and avoid this. This is
(will be) part of my patch for caching</FONT></DIV>
<DIV><FONT size=2 face=Consolas>constexpr function evaluations, which I am ready
to post when all the groundwork</FONT></DIV>
<DIV><FONT size=2 face=Consolas>patches are in. Lets take it out for now,
we can discuss it again when I can</FONT></DIV>
<DIV><FONT size=2 face=Consolas>demonstrate its purpose more obviously.
Maybe you will suggest an alternative,</FONT></DIV>
<DIV><FONT size=2 face=Consolas>in which case this change is not
needed.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>(As a side note, ForceEmitDiagnostics is
expected only to be used as a wrapper<BR>around a single diagnostic, where it is
determined that whatever suppressions<BR>might otherwise be in place, this
diagnostic must be emitted anyway.)</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>> The rest of this part looks
fine.<BR>> <BR>> Part 5: Implement conditional diagnostic checking,
moving<BR>> from
post-processing of source file to using
the<BR>> CommentHandler
interface<BR>><BR>> The names Seen and Unseen are quite opaque to me.
FilesWithDiagnostics and<BR>> FilesWithDirectives would be much
clearer.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Ok, will do. Your names are certainly
better.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>> I also don't yet understand why we would
get into the situation where we need<BR>> the special-case code to scan the
FilesWithDiagnostics which we didn't find<BR>> directives in. Is this related
to modules / PCH / PPH? Can you point out an<BR>> example test where this
comes up?</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>In short, yes, the problems relate to modules
and pre-compiled headers, and<BR>as in the case of ARCMT.cpp, where there is a
separate parse process.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>To quote from my other mail on
this:</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>"In an ideal world there should be no mismatch
at the end of processing <BR> between the "seen" and "unseen" lists.
However, this is still not the<BR> case (my implementation while
arguably better is still not perfect)<BR> since *some* synthesised include
files are still not "seen". In the<BR> case of ARCMT.cpp, I have
fixed this but other cases still exist.<BR> Therefore, I have a backup
which scans the unseen list and ensures that<BR> these files are processed
at the end even though they were missed first<BR> time around. In the
end, we can still guarantee that all source files<BR> have been
seen."</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>I believe there should be ways of tidying up
these loose ends (for want of a<BR>better term!), but I am hard-pressed to find
the time to do this right now. In<BR>a month's time I should be able to look
into it, but I really don't want this</FONT></DIV>
<DIV><FONT size=2 face=Consolas>to be a hold point to these patches. The
position I have left it in is safe:</FONT></DIV>
<DIV><FONT size=2 face=Consolas>files falling through the first net are caught
in the second -- still better</FONT></DIV>
<DIV><FONT size=2 face=Consolas>than the current situation where there is only
one net, which has a few holes</FONT></DIV>
<DIV><FONT size=2 face=Consolas>in it already!</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Can we put a FIXME marker in the source at this
point? When fixed, I would<BR>still suggest that under a debug build
(#ifndef NDEBUG) we still scan through<BR>the two lists and assert on
mismatch.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>I haven't got my code in front of me, so I can't
give you exact examples of<BR>test case failures, but you can always just insert
an assert, and see where<BR>the errors occur... They will occur in
test/ARCMT, test/Modules and test/PCH.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>> I also don't like the coupling between
DiagnosticConsumer and CommentHandler<BR>> which the getCommentHandler method
introduces. This seems to exist only to<BR>> support part 6, so I wonder if
there's a better way to approach that.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Yes, this is in support of part 6. I am
open to suggestions for a better<BR>approach, but it does need to be available
somehow to a sub-class of<BR>DiagnosticConsumer. Introducing
getCommentHandler seemed the most obvious<BR>way.<BR> <BR>> Part 6:
Update CaptureDiagnosticConsumer in ARCMT.cpp
to<BR>> also capture and
forward the comment lines,
as<BR>> well as the
diagnostics<BR>><BR>> We need this to fix ARCMT tests broken by part 5,
right? I don't know anything<BR>> about this code, so hopefully someone else
will look over these changes.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>Actually, this *doesn't* fix any broken tests
since these tests are not broken<BR>by my part 5 patch. This patch is
provided to move away from needing the scan<BR>of unseen files in that patch, as
is both our wishes.<BR> <BR>> Part 7: Test cases<BR>><BR>> These
should be split up to go with the related code changes above.</FONT></DIV>
<DIV> </DIV>
<DIV><FONT size=2 face=Consolas>You could link them then to the part 5 patch,
since this is the one which<BR>requires the changes to be made. All the
other patches do not introduce any<BR>regressions.<BR></FONT></DIV>
<DIV><FONT size=2 face=Consolas>I will update my patches according to your
comments, and repost them tonight.</FONT></DIV>
<DIV><FONT size=2 face=Consolas></FONT> </DIV>
<DIV><FONT size=2 face=Consolas>Thanks for all your help.</FONT></DIV>
<DIV><FONT size=2 face=Consolas></FONT> </DIV>
<DIV><FONT size=2 face=Consolas>Andy</FONT></DIV>
<DIV><FONT size=2 face=Consolas></FONT> </DIV>
<DIV><FONT size=2 face=Consolas> </DIV></FONT></BODY></HTML>