[PATCH] D74795: Make diagnostic reporting more robust in presence of corrupt PCH data.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Thanks for working on this!  I have a couple of comments inline.



================
Comment at: clang/include/clang/Basic/Diagnostic.h:918-927
   /// The ID of the current diagnostic that is in flight.
   ///
   /// This is set to std::numeric_limits<unsigned>::max() when there is no
   /// diagnostic in flight.
   unsigned CurDiagID;
 
+  /// The ID of the current delayed diagnostic being reported.
----------------
`CurDelayedDiagID` has a different kind of "in-flight" lifetime than `CurDiagID`.  I think the comment here should be more explicit about what precisely the variable is doing, and perhaps it should have another name.  Do we actually need the ID kept as state?  If not, this would be simpler as:
```
bool IsReportingDelayedDiag = false;
```

Also, a couple of nit-picks:
- Periods at the end of sentences in comments.
- You seem to have dropped out of doxygen comments (`//` instead of `///`).


================
Comment at: clang/lib/Basic/Diagnostic.cpp:160-161
 void DiagnosticsEngine::ReportDelayed() {
-  unsigned ID = DelayedDiagID;
+  CurDelayedDiagID = DelayedDiagID;
   DelayedDiagID = 0;
+  Report(CurDelayedDiagID) << DelayedDiagArg1 << DelayedDiagArg2
----------------
If you take my suggestion, then you wouldn't need to change this line, just add:
```
IsReportingDelayedDiag = true;
```
but would these assertions be valid after your change?
```
assert(DelayedDiagID && "Called without a delayed diagnostic?");
assert(!IsReportingDelayedDiag &&
       "Called while reporting another delayed diagnostic?");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74795/new/

https://reviews.llvm.org/D74795