[cfe-commits] [Patch 1 of 4] -verify fixes (stage 2)

[Jordan Rose]

Hi, Andy. I've been delaying my review of this patch because it doesn't exactly sit well with me, but I think it will be the right way to go. Comments inline.


On Jul 17, 2012, at 9:18 AM, Andy Gibbs wrote:

> Hi,
> 
> The next stage of changes for -verify is now ready, and I am submitting
> them for comments.  These four patches pick up where the last set left
> off, and fix the remaining issues as discussed previously.
> 
> Unfortunately, this time the patches did not easily split out, but I have
> attempted to do so in a logical way for easier review.  But it does mean,
> however, that all patches should be committed together since they do rely
> on each other.
> 
> Attached to this post is part 1.  In this part, the main set of changes
> to VerifyDiagnosticConsumer are made, and this consists of:
> 
> 1. Renaming FilesWithDirectives to FilesParsedForDirectives which better
>   describes its actual use (sorry!).
> 
> 2. As a result, rolled back a change made by Jordan relating to the old
>   FilesWithDirectives where files were only added if directives were
>   found.

+
+#ifndef NDEBUG
+  if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
+    FilesParsedForDirectives.insert(E);
+#endif

I'm still not so happy about this, because a file with no comments will look the same as a PCH or whatever. And it shouldn't be necessary with (3) -- every file with comments will have been seen by PPCallbacks. Did you have a specific motivating example for this?


> 3. Integrated PPCallbacks so that FilesParsedForDirectives also includes
>   files which have been seen but have no visible directives (e.g. they are
>   all inside skipped #if blocks).

More Preprocessor awkwardness, but okay. I'd like to see the Preprocessor made more robust some day so that it's acceptable to add and remove callbacks even once it's been initialized.


> 4. Ensure that repeated BeginSourceFile calls are handled correctly, as is
>   a mismatch between calls of BeginSourceFile and EndSourceFile (it turns
>   out, the latter is often called twice while the former only once, but
>   also it is possible for BeginSourceFile to be called more than once too).

This worries me. If we have unbalanced BeginSourceFile and EndSourceFile calls, that might be a bug. Do you know specific circumstances where this is the case?


> 5. No longer add directives found during post-processing, but generate an
>   error if such directives *are* found (which they shouldn't be!).

+    // If a directive has been found but we're not interested
+    // in storing the directive information, return now.
+    if (!DL) return true;

Nitpick via LLVM coding style: "return true;" should be on the next line.


> 6. Only do the post-processing check in a debug/asserts build now since it
>   is not otherwise necessary, and in an optimised build we can save the
>   cost of this check.


+/// FoundExpectedDiags - Lex the main source file to find all of the
+///  expected errors and warnings.

Nitpicking on the name: I don't immediately parse this as a predicate, and even then it's doing work. Coming from a Cocoa background, I'd prefer didFindExpectedDiags, but even just findExpectedDiags would work too for me. Or findDirectives.


+#ifndef NDEBUG
+    // In a debug build, scan through any files that may have been missed
+    // during parsing and issue a fatal error if diagnostics are contained
+    // within these files.  This should not occur, and if it does then it
+    // implies that (some) directives are not being made available during
+    // normal parsing and first it should be checked why this is the case.

The sentence here is hard to parse. Maybe break out the last clause? I'm not even sure what it means myself -- something like "It's possible these files are precompiled in some way." or similar.


+      if (FoundExpectedDiags(*CurrentPreprocessor, *I))
+        llvm::report_fatal_error(Twine("-verify directives found after rather"
+                                       " than during normal parsing of ",
+                                 StringRef(E ? E->getName() : "(unknown)")));

Is it possible to get a test case for this, or is it really impossible? If the latter, you might as well just assert(!FoundExpectedDiags(*CurrentPreprocessor, *I)). 

Jordan