We already do this with things like llvm::DenseMapInfo, I don't know why
I was doing this with format_provider. But this should be more
consistent, and slightly better for not entering another library's
namespace.
I was suggesting this because `FloatingPoint` is pretty long. `int` and
`float` should be familiar abbreviations. `unsigned` should be familiar
to developers too, but `UnsignedInt` still feels usefully clearer for
the additional chars.
This gets to a lifetime subtlety, particularly with things like the
sorting diagnostic consumer that delay output. In order to reduce the
chance of accidental references, disallow StringRef in the diagnostics.
For example:
```
./toolchain/diagnostics/diagnostic_emitter.h:162:5: error: static_assert failed due to requirement '!std::is_same_v<llvm::StringRef, llvm::StringRef>' "Use std::string or llvm::StringLiteral for diagnostic lifetimes."
static_assert(
^
toolchain/check/convert.cpp:477:11: note: in instantiation of member function 'Carbon::Internal::DiagnosticBase<std::string, std::string, llvm::StringRef>::DiagnosticBase' requested here
CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
^
./toolchain/diagnostics/diagnostic_emitter.h:47:7: note: expanded from macro 'CARBON_DIAGNOSTIC'
::Carbon::Internal::DiagnosticBase<__VA_ARGS__>( \
^
```
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Per [#toolchain
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1176632520834560211)
We'd at one point been trying to put `[[nodiscard]]` everywhere, but
then we stopped because it had felt verbose without finding many issues
(plus, people plain forgot to add it). Some history in #888.
Since newer code gets added without it, we now have code like:
```
auto GetLineInfo(Line line) -> LineInfo&;
[[nodiscard]] auto GetLineInfo(Line line) const -> const LineInfo&;
auto AddLine(LineInfo info) -> Line;
auto GetTokenInfo(Token token) -> TokenInfo&;
[[nodiscard]] auto GetTokenInfo(Token token) const -> const TokenInfo&;
auto AddToken(TokenInfo info) -> Token;
[[nodiscard]] auto GetTokenPrintWidths(Token token) const -> PrintWidths;
```
Here, the lack of `[[nodiscard]]` doesn't mean anything: for example,
`GetLineInfo` should not have its result discarded if it's called. But
the mix could be confusing for readers.
As a resolution, remove the attribute. `[[nodiscard]]` should be treated
like other attributes going forward, which essentially means "avoid in
general, add a comment to explain why the attribute is needed" rather
than use-as-default.
When originally switching to the table dispatch approach we discussed
that it'd be nice to disentangle the monolithic symbol lexing routine
with this as we'll typically have fairly precise dispatch. This is
especially true for grouping symbols, which in Carbon are all
constructively one-character (at this point).
I think this provides a substantial improvement to the clarity of the
code by disentangling the different paths. It also allowed a bunch of
simplifications / clarifications to exactly what the behavior with
closing invalid groups actually involves currently.
This was initially motivated by code organization improvements, and any
performance wins were speculative. However, when benchmarking it
surfaced a problem that hadn't been clear -- we're generating too many
distinct functions here, and the table-based dispatch slows down in the
face of that.
So this PR also includes a fix for that, removing the template-generated
fan-out of dispatch functions for distinct symbols. Instead, we have
a dedicated table to translate one character into the token kinds. This
seems to work quite well, avoiding the huge branch-y structure and just
do fairly cheap table translation & dispatch for all one-character
symbols. Building the table requires the token kinds to be default
constructable, so this also enables that and arranges for the zero-value
kind to be the error kind.
Combined, this is a modest speedup for *non* grouping symbols (3-4%,
a bit noisy). And in some cases it is a huge speedup for grouping
symbols (>10%).
Raw benchmark data with 20 runs before/after -- despite the # of runs,
the grouping symbols benchmarks were frustratingly noisy in non-uniform
ways that couldn't fully be accounted for here. Still, this seems like
an overall improvement.
```
BM_RandomSource 7.98ms ± 2% 7.73ms ± 3% -3.12% (p=0.000 n=18+19)
BM_GroupingSymbols/1/0/0 5.90ms ± 2% 5.82ms ± 4% -1.38% (p=0.001 n=20+20)
BM_GroupingSymbols/2/0/0 5.21ms ± 2% 5.15ms ± 2% -1.14% (p=0.002 n=20+18)
BM_GroupingSymbols/3/0/0 4.42ms ± 2% 4.34ms ± 2% -1.87% (p=0.000 n=19+18)
BM_GroupingSymbols/4/0/0 4.29ms ± 2% 4.38ms ± 5% ~ (p=0.297 n=17+20)
BM_GroupingSymbols/8/0/0 5.09ms ±10% 5.10ms ± 7% ~ (p=0.919 n=18+20)
BM_GroupingSymbols/16/0/0 6.35ms ± 8% 6.29ms ± 6% ~ (p=0.201 n=20+20)
BM_GroupingSymbols/32/0/0 9.88ms ± 2% 9.83ms ± 1% ~ (p=0.167 n=18+20)
BM_GroupingSymbols/0/1/0 5.12ms ± 2% 5.01ms ± 2% -2.14% (p=0.000 n=20+19)
BM_GroupingSymbols/0/2/0 4.01ms ± 2% 3.93ms ± 4% -2.03% (p=0.000 n=20+19)
BM_GroupingSymbols/0/3/0 2.92ms ± 3% 2.81ms ± 2% -3.87% (p=0.000 n=20+19)
BM_GroupingSymbols/0/4/0 2.61ms ± 3% 2.47ms ± 2% -5.30% (p=0.000 n=20+18)
BM_GroupingSymbols/0/8/0 1.77ms ± 3% 1.61ms ± 2% -8.91% (p=0.000 n=18+19)
BM_GroupingSymbols/0/16/0 1.41ms ± 3% 1.16ms ± 4% -17.66% (p=0.000 n=20+20)
BM_GroupingSymbols/0/32/0 1.10ms ± 2% 0.92ms ± 3% -16.36% (p=0.000 n=20+17)
BM_GroupingSymbols/0/0/1 5.09ms ± 2% 5.03ms ± 3% -1.11% (p=0.001 n=20+18)
BM_GroupingSymbols/0/0/2 4.01ms ± 2% 3.91ms ± 2% -2.67% (p=0.000 n=20+18)
BM_GroupingSymbols/0/0/3 2.93ms ± 3% 2.81ms ± 2% -4.23% (p=0.000 n=20+19)
BM_GroupingSymbols/0/0/4 2.59ms ± 2% 2.48ms ± 3% -4.48% (p=0.000 n=20+19)
BM_GroupingSymbols/0/0/8 1.75ms ± 1% 1.62ms ± 3% -7.65% (p=0.000 n=17+19)
BM_GroupingSymbols/0/0/16 1.40ms ± 2% 1.15ms ± 3% -17.67% (p=0.000 n=19+20)
BM_GroupingSymbols/0/0/32 1.10ms ± 2% 0.92ms ± 3% -15.91% (p=0.000 n=20+19)
BM_GroupingSymbols/32/1/0 9.62ms ± 2% 9.65ms ± 2% ~ (p=0.654 n=18+20)
BM_GroupingSymbols/32/2/0 9.41ms ± 2% 9.37ms ± 2% ~ (p=0.095 n=20+19)
BM_GroupingSymbols/32/3/0 9.13ms ± 2% 9.13ms ± 3% ~ (p=0.687 n=19+20)
BM_GroupingSymbols/32/4/0 8.93ms ± 1% 8.87ms ± 2% -0.69% (p=0.010 n=20+18)
BM_GroupingSymbols/32/8/0 8.15ms ± 2% 8.14ms ± 3% ~ (p=0.729 n=19+19)
BM_GroupingSymbols/32/16/0 7.04ms ± 3% 6.92ms ± 1% -1.71% (p=0.000 n=20+18)
BM_GroupingSymbols/32/32/0 5.48ms ± 2% 5.38ms ± 3% -1.81% (p=0.000 n=20+20)
BM_GroupingSymbols/32/32/1 5.39ms ± 2% 5.29ms ± 2% -1.87% (p=0.000 n=19+19)
BM_GroupingSymbols/32/32/2 5.34ms ± 2% 5.21ms ± 1% -2.45% (p=0.000 n=20+18)
BM_GroupingSymbols/32/32/3 5.27ms ± 3% 5.16ms ± 2% -2.18% (p=0.000 n=20+19)
BM_GroupingSymbols/32/32/4 5.21ms ± 2% 5.10ms ± 3% -2.11% (p=0.000 n=19+20)
BM_GroupingSymbols/32/32/8 4.98ms ± 2% 4.83ms ± 2% -2.85% (p=0.000 n=19+19)
BM_GroupingSymbols/32/32/16 4.55ms ± 2% 4.45ms ± 2% -2.25% (p=0.000 n=18+20)
BM_GroupingSymbols/32/32/32 3.95ms ± 2% 3.84ms ± 2% -2.98% (p=0.000 n=19+20)
```
The big change is to make the lexer helpers have internal linkage,
making all of them easy to inline into single call sites.
Looking at the profile showed several other cases of unfortunate
out-of-line functions. Two were due to the code size produced for checks
-- those are switched to `DCHECK`s to remove that code from optimized
builds. The loss of coverage seems minor.
A last one was closing open groups. This was a surprising routine to be
hot, but it the paths to discover "nothing to do here" were intertwined
into the code. This PR extracts this common trace into a separate
function that delegates to the looping recovery path. This lets the hot
path inline easily.
At this point, for a large lexing benchmark I'm using, 50% of the time
is in the identifier hash table at this point. The remaining
improvements are to actually make some of the hot routines like symbol
lexing and comment lexing faster.
Continuing with #3070. Just a dir and file rename (only prefix change is
lexer_file_test). Everything in the lex dir should be marked as a move.
Note, I think this closes#3070. There may still be further cleanup
later, but the organizational changes suggested there are being
completed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Jon Ross-Perkins
josh11b