rsmith added a comment.
In D137751#3919162 <https://reviews.llvm.org/D137751#3919162>, @alexfh wrote:
> I wonder whether we can include SLoc usage information into `-print-stats`
> output?
Yeah, we could extend the source manager information there with something like
this. It's a bit awkward to reuse this implementation because it's geared
around producing notes, but it should be fairly straightforward to factor out
some common code and extend `-print-stats` with it in a follow-up change.
================
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;
+
----------------
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.
================
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 &&
----------------
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.
================
Comment at: clang/lib/Basic/SourceManager.cpp:2251
+ uint64_t CountedSize = 0;
+ for (int IDIndex = -(int)LoadedSLocEntryTable.size() - 1;
+ IDIndex < (int)LocalSLocEntryTable.size(); ++IDIndex) {
----------------
shafik wrote:
> alexfh wrote:
> > Could you add a comment about the meaning of negative IDIndex values?
> Maybe make `-(int)LoadedSLocEntryTable.size()` a named variable may help
> readability as well. Perhaps also for `(int)LocalSLocEntryTable.size()` as
> well.
Refactored to make it clearer what's going on, added comments.
================
Comment at: clang/lib/Basic/SourceManager.cpp:2281-2282
+ SortedUsage.reserve(Usage.size());
+ for (auto It = Usage.begin(); It != Usage.end(); ++It)
+ SortedUsage.push_back(It);
+ auto Cmp = [](UsageMap::iterator A, UsageMap::iterator B) {
----------------
aaron.ballman wrote:
> llvm::copy?
This is forming a vector of iterators, not a vector of values. Hm, but... I
think I can express this better with `MapVector` and avoid this copy in the
process.
================
Comment at: clang/lib/Lex/Pragma.cpp:1187
+ // specifically report information about.
+ uint64_t MaxNotes = (uint64_t)-1;
+ Token ArgToken;
----------------
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`.
================
Comment at: clang/test/Misc/sloc-usage.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
----------------
aaron.ballman wrote:
> Should we do anything special about source locations from the command line,
> or do those not contribute source locations? (e.g., `-D` or `-U`, force
> include file, etc flags)
>
> (Testing a forced include would be interesting regardless, just to ensure we
> catch those the same as we do an `#include`.)
For `-D` etc, we get output that looks like this:
```
<built-in>:1:1: note: file entered 2 times using 14778B of space
# 1 "<built-in>" 3
^
```
... where the `<built-in>:1:1` is the location of the first time we enter a
`FileID` with a null corresponding `const FileEntry*`. I think these will all
correspond to predefines and similar things, so the `<built-in>:1:1` source
location seems pretty reasonable.
I've added a test for `-include`.
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
