Page MenuHomePhabricator

[NFC] Refactor DiagnosticRenderer to use FullSourceLoc

Authored by christof on Apr 5 2017, 7:29 AM.



Move the DiagnosticRenderer and its dependents to using FullSourceLocs
instead of a SourceLocation and SourceManager pointer. The changeset is
rather large but entirely mechanical.

This is step one to allow DiagnosticRenderer to take either
llvm::SMLocs or clang::SourceLocations.

Diff Detail


Event Timeline

sanwou01 created this revision.Apr 5 2017, 7:29 AM
sanwou01 updated this revision to Diff 95541.Apr 18 2017, 2:43 AM

Rebased and clang-formatted.

sanwou01 edited the summary of this revision. (Show Details)Apr 18 2017, 2:44 AM
rovka accepted this revision.Apr 20 2017, 5:20 AM
rovka added a subscriber: rovka.

I don't see anything wrong with this, I think you can commit it in a couple of days if nobody comes up with a reason why the DiagnosticRenderer shouldn't use FullSourceLoc.

This revision is now accepted and ready to land.Apr 20 2017, 5:20 AM

Hi Diana,

Thanks! Will do :)


rnk added inline comments.Apr 20 2017, 12:55 PM
336 ↗(On Diff #95541)

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;
25 ↗(On Diff #95541)

This doesn't seem necessary, you aren't defining any free functions, right?

139 ↗(On Diff #95541)

I think Diag.getLocation() is already a FullSourceLoc, no need to check it.

512 ↗(On Diff #95541)

This seems inefficient, it wastes space on SourceManager pointers that will all be the same.

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

336 ↗(On Diff #95541)

That makes sense, and seems like a good sanity check. I'll run regressions to make sure nothing insane is going on anywhere.

25 ↗(On Diff #95541)

I might have done in a previous iteration, resulting in linker errors which this fixed. I'll try again without. :)

139 ↗(On Diff #95541)

Quite right, good catch!

512 ↗(On Diff #95541)

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?

christof commandeered this revision.Jun 8 2017, 9:07 AM
christof added a reviewer: sanwou01.
christof added a subscriber: christof.

This refactoring was ready to land some time ago, except for a few small details. It's a shame if we don't commit this refactoring, so with @sanwou01
his agreement, I'm taking over this patch.

christof updated this revision to Diff 101933.Jun 8 2017, 9:36 AM
christof marked 6 inline comments as done.

Rebased onto current HEAD and addressed the comments from Reid.

@rnk can you let me know if you are happy with the changes?
@rovka I assume the rebase did not change your mind about the patch. But please let me know if you have new reservations.


512 ↗(On Diff #95541)

We indeed waste that space, but it is 8 times a pointer while we printing diagnostics. I don't think that this inefficiency is is of any concern, especially not considering the amount of buffering that goes on during printing anyway.

I'll go ahead and commit this on the grounds that it was already approved and the changes I made were the last nit-picks of @rnk

This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.

This is removed. [-Wdocumentation]

Pruned in rL306511.

christof marked an inline comment as done.Jun 28 2017, 1:30 AM

Sorry for missing the API comments. Thanks for the fix chapuni :-)