This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Refactor DiagnosticRenderer to use FullSourceLoc
ClosedPublic

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

Details

Summary

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

Repository
rL LLVM

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 :)

Sanne

rnk added inline comments.Apr 20 2017, 12:55 PM
include/clang/Basic/SourceLocation.h
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;
}
lib/Basic/SourceLocation.cpp
25 ↗(On Diff #95541)

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

lib/Frontend/DiagnosticRenderer.cpp
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.

include/clang/Basic/SourceLocation.h
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.

lib/Basic/SourceLocation.cpp
25 ↗(On Diff #95541)

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

lib/Frontend/DiagnosticRenderer.cpp
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.

Thanks

lib/Frontend/DiagnosticRenderer.cpp
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.
cfe/trunk/include/clang/Frontend/DiagnosticRenderer.h
131

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 :-)