[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

Wed Nov 16 05:24:18 PST 2022

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