aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM! Please add a release note when landing so folks know about the improvements here. Thanks! ================ Comment at: clang/include/clang/Basic/SourceManager.h:1695-1696 + // Produce notes describing the current source location address space usage. + void noteSLocAddressSpaceUsage(DiagnosticsEngine &Diag, + unsigned MaxNotes = 50) const; + ---------------- rsmith wrote: > aaron.ballman wrote: > > Not that I'm opposed, but how did you arrive at 50? > I was aiming for a number that's small enough that the diagnostic won't run > for pages (will fit into a relatively small scroll buffer) but large enough > that it should capture problematic usage. > > But, you know what, we should be consistent. A default of 32 would match our > default limit for notes in constexpr and template backtraces; let's use that. > Plus powers of 2 are the best arbitrary numbers. Thanks, that makes more sense! Someday (not today) we might want to consider making this a named constant and using it rather than magic numbers. ================ Comment at: clang/lib/Basic/SourceManager.cpp:675 LocalSLocEntryTable.push_back(SLocEntry::get(NextLocalOffset, Info)); + // TODO: Produce a proper diagnostic for this case. assert(NextLocalOffset + Length + 1 > NextLocalOffset && ---------------- rsmith wrote: > aaron.ballman wrote: > > Are you planning to do this as part of this patch, or is this more of an > > aspirational FIXME for the future? > This is aspirational. I think it'll be a lot of work to make all the > transitive callers of this be able to handle it failing. SGTM, thanks! ================ Comment at: clang/lib/Lex/Pragma.cpp:1187 + // specifically report information about. + uint64_t MaxNotes = (uint64_t)-1; + Token ArgToken; ---------------- rsmith wrote: > aaron.ballman wrote: > > `~0ULL`? > That would be `(unsigned long long)-1`, which isn't necessarily the same > value. > > But I think we can do better than forming a large unsigned number here; > changed to use `Optional`. Even better! Thanks CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137751/new/ https://reviews.llvm.org/D137751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
