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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
