[cfe-commits] r161650 - in /cfe/trunk: include/clang/Frontend/VerifyDiagnosticConsumer.h lib/ARCMigrate/ARCMT.cpp lib/Frontend/VerifyDiagnosticConsumer.cpp test/ARCMT/verify.m test/ASTMerge/function.c test/Frontend/verify.c test/Frontend/verify2.c test/Frontend/veri

[Jordan Rose]

Looks fairly good. A couple comments:


+  typedef llvm::SmallDenseMap<FileID, FileStatus, 4> FilesProcessedMap;
+  FilesProcessedMap FilesProcessed;

I would make this a bit bigger. I know this only really matters for the -verify cases in our regression tests, but I can see some of those that have more than 4 source files.


+  const FileEntry* FE = SM.getFileEntryForID(FID);

Please attach the star to the variable name. :-)


+  std::pair<FilesProcessedMap::iterator, bool> I =
+    FilesProcessed.insert(std::make_pair(FID, FileStatus(FE, MarkParsed)));
+
+  // If already in map, check for matching FileEntry and mark as parsed,
+  // if required.
+  if (!I.second) {
+    assert(I.first->second.getFile() == FE && "Mismatched FileEntry!");
+    if (MarkParsed)
+      I.first->second.setParsedForDirectives();
+  }

I find this very hard to read. More temporaries, please? It might also be nicer to use llvm::tie here.


+        for (FilesProcessedMap::iterator J = FilesProcessed.begin();
+               J != End; ++J) {

I don't like this inner loop. Can we avoid it with a set of FileEntry pointers? (Either a set of parsed files or a set of unparsed files, not sure which would be easier.)

Stepping back, maybe that's a reason to go back to having two sets.

---When you enter a file from the preprocessor---
remove from unparsed set
add to parsed set

---When you get a diagnostic---
if (not in parsed set)
  add to unparsed set

---When you check at the end---
if (unparsed set not empty)
  build set of paresd FileEntry pointers
  iterate through unparsed set

I'm not sure if this is an overall simpler approach, but it makes more sense to me. What do you think?

Jordan


On Aug 14, 2012, at 10:02 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> Hi,
> 
> Attached is an improvement to the backup checking algorithm that better
> supports cling's use-case.
> 
> In terms of general operation (i.e. in terms of clang's test-suite), the
> functionality is unchanged.  Instead of maintaining two separate lists
> for files with diagnostics and those parsed for directives, a single list
> is now held, and implemented as a DenseMap with the key as FileID and
> the value as a pair of FileEntry* and a bool indicating whether the file
> has been parsed for directives or not.
> 
> It is important, as stated elsewhere in this thread, to keep FileEntry*
> so that FileID aliases can be detected (as is necessary with PCHs, for
> example).
> 
> The testing I was able to do with cling suggests that the problem there
> is now solved.
> 
> Jordan, ok to commit?
> 
> Cheers
> Andy
> <verify.diff>