[PATCH] D31709: [NFC] Refactor DiagnosticRenderer to use FullSourceLoc

[Sanne Wouda via Phabricator via cfe-commits]

sanwou01 added a comment.

Thanks for your comments Reid. Please find my responses inline. I'll spin a new patch addressing your comments soonish.



================
Comment at: include/clang/Basic/SourceLocation.h:336
 
+  bool hasManager() const { return SrcMgr != nullptr; }
   /// \pre This FullSourceLoc has an associated SourceManager.
----------------
rnk wrote:
> SrcMgr is only non-null when the location is invalid, right? Can you do something like:
>   bool hasManager() const {
>     bool R = SrcMgr != nullptr;
>     assert(R == isValid() && "FullSourceLoc has location but no manager");
>     return R;
>   }
That makes sense, and seems like a good sanity check. I'll run regressions to make sure nothing insane is going on anywhere.


================
Comment at: lib/Basic/SourceLocation.cpp:25
 
+namespace clang {
 void PrettyStackTraceLoc::print(raw_ostream &OS) const {
----------------
rnk wrote:
> This doesn't seem necessary, you aren't defining any free functions, right?
I might have done in a previous iteration, resulting in linker errors which this fixed. I'll try again without. :)


================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:139
 void DiagnosticRenderer::emitStoredDiagnostic(StoredDiagnostic &Diag) {
-  emitDiagnostic(Diag.getLocation(), Diag.getLevel(), Diag.getMessage(),
-                 Diag.getRanges(), Diag.getFixIts(),
-                 Diag.getLocation().isValid() ? &Diag.getLocation().getManager()
-                                              : nullptr,
-                 &Diag);
+  emitDiagnostic(
+      Diag.getLocation().isValid()
----------------
rnk wrote:
> I think `Diag.getLocation()` is already a FullSourceLoc, no need to check it.
Quite right, good catch!


================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:512
   // Produce a stack of macro backtraces.
-  SmallVector<SourceLocation, 8> LocationStack;
+  SmallVector<FullSourceLoc, 8> LocationStack;
   unsigned IgnoredEnd = 0;
----------------
rnk wrote:
> This seems inefficient, it wastes space on `SourceManager` pointers that will all be the same.
True, but it does make the rest of this function more readable. I'd prefer to leave it as is. Maybe just reducing the SmallVector size to, say, 4, to take up less stack space?


https://reviews.llvm.org/D31709