[cfe-commits] [cfe-dev] Patch for review: enhancements/fixes to -verify (resubmission)

[Andy Gibbs]

On Monday, June 18, 2012 4:53 AM, Richard Smith wrote:
> OK, I think you've made a good case for all these features. However,
> you are much more likely to get them reviewed if you can provide
> separate patches for each feature.

No problem.  I have split it up into 7 parts and posted them
to cfe-commits at cs.uiuc.edu:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059586.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059587.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059588.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059589.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059590.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059591.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059592.html

>> 3. It could happen that the current implementation would not gather
>> all the expected-* checks from included header files since it
>> only considered the main file and one other FileID record. The
>> new implementation considers all FileID records, marking them
>> "unseen" if a diagnostic occurs with them, and "seen" if a
>> expected-* check is found within them. At the end, if there
>> exists any files that are "unseen" and not "seen", then these
>> are parsed again for expected-* checks. This should limit the
>> likelihood of missed expected-* checks, and did reveal an error
>> in test/Modules/on-demand-build.m test-case.
>
> I didn't follow this. It seems to me that we should either (a) only
> consider expected-* comments we see when preprocessing the main source
> file, or (b) consider all expected-* comments we see in any file (and
> the latter makes more sense to me). When does the need for re-parsing
> arise?

Alright, not so well explained!  In the old implementation, where
synthesised include files are used (for example, in modules or pre-
compiled headers) the implementation picked up "one other" FileID and
additionally processed this.  The logic was not perfect and it could
(and did in one test-case) lead to a mismatch of expected-* checks and
actual diagnostics.  The new implementation betters this slightly by
having a "seen" and "unseen" list: "seen" meaning it has extracted
expected-* checks from this file, "unseen" meaning that diagnostics
have been generated by this file.  In an ideal world there should be
no mismatch at the end of processing between the "seen" and "unseen"
lists.  However, this is still not the case (my implementation while
arguably better is still not perfect) since *some* synthesised
include files are still not "seen".  In the case of ARCMT.cpp, I have
fixed this but other cases still exist.  Therefore, I have a backup
which scans the unseen list and ensures that these files are processed
at the end even though they were missed first time around.  In the
end, we can still guarantee that all source files have been seen.

A perfect solution, therefore, shouldn't need this backup check.  When
I have some more time, I can devote it to pinning down these last
remaining cases and ensure that comments are bubbled up correctly to
VerifyDiagnosticConsumer.

In the meantime, I hope you will agree that the new implementation
is otherwise correct and worthy of being committed?

Many thanks again
Andy