Rules executed by bazel don't necessarily have the right environment to
find the symbolizer, which was the intent of `cc_env` setting
`LLVM_SYMBOLIZER_PATH`. So far, this has kind of been a case-by-case
fix, but every so often I'm trying to debug a crash in a test that
doesn't provide it. Rather continuing down this route, instead add
drop-in wrappers for cc rules so that it's hard to forget.
Note `bazel/cc_rules` is intended to mirror `bazel/carbon_rules` and
`bazel/cc_toolchains`, rather than `@rules_cc`.
AFAICT there isn't a great way to add this as a default for the `bazel
run` environment. It's not typically going to be set on its own,
forwarding `$PATH` would be too broad, and the [action
`env_sets`](https://bazel.build/docs/cc-toolchain-config-reference#using-action-config)
I think are not quite what we need (I think those don't include output
execution, only compilation).
This is trying to document the status quo. Note
https://github.com/carbon-language/carbon-lang/pull/4497 placed
restrictions on a bit, and this is in part flowing back to restrictions
on both NodeId and ImportIRInstId limits.
Given the namespacing of `Diagnostics` in #5173, now we can use
`DiagnosticEmitter` for phase-specific emitters. This is consistent with
how we do `Context`, and also check had started this with
`DiagnosticBuilder` in anticipation of the namespacing.
Also renames `Emitter::DiagnosticBuilder` to `Emitter::Builder` for
consistency with other `Diagnostics` entities.
In check, I'm still splitting `DiagnosticEmitterBase` and
`DiagnosticEmitter` just to keep the emitter definition separate from
the context.
Also cleans up some incorrect check diagnostic emitter dependencies in
lower.
I was thinking about this after `seq` changes in #5182, and looked for
other uses that might be replaceable. Here's the resulting cleanup
around `seq`:
- Switch to `enumerate` or `zip` when possible.
- `int _` -> `auto _` (it's typically a `size_t`, but there's no reason
to cast when unused)
- Fix a case of cast style `(size_t)...` -> `static_cast<size_t>(...)`
- Switch `(void)close_children_count` to `[[maybe_unused]]`
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
We had a long discussion of this, so trying to document what seems to be
the conclusion... and also clean up the exceptions that I could find.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
In line with the proposal in #4682, this changes the array syntax to be
array(T, N). `array` is a builtin keyword which must be followed by
parens containing two expressions and a separating comma.
The array type expression is still fully builtin, it does not forward to
a Core.Array library type yet. It merely adds the `ArrayType`
instruction, as was done with the previous syntax.
Followup work will change the instruction to reference to Core.Array,
once the library type exists and can be used directly.
---------
Co-authored-by: zygoloid <richard@metafoo.co.uk>
This adds support for choice types at a similar level to that of a C
enum, where each alternative has a name but no additional
data/parameters attached to it. We generate a TODO diagnostic if
parameters are specified.
Because there's no extra data, the storage is a simple unsigned integer
discriminant of the smallest possible size.
A choice without any alternatives is not constructible. A choice with a
single alternative is, and has an empty tuple in place of a discriminant
since it has only one state. The empty tuple is used to make the class
non-constructible. This can be improved.
Each alternative is turned into a let binding on the choice that is a
value of the choice with that alternative set as the active one in the
discriminant. This isn't possible to write in user code with a class
right now, since the let binding has the same type as the choice
(which is a class) it is within. It's possible to generate it in semir
however by adding the binding after the class is marked complete.
Change representation of package names from `IdentifierId` to
`PackageNameId`, and add a special value `PackageNameId::Core` for the
Core package. Add a `Core` expression to name the Core package, and
support for parsing the `Core` keyword in `package` and `import`
declarations.
For now, I've made no changes to instruction fingerprinting or name
mangling. This means that fingerprints and mangled names will collide
between names in the `Core` package and names in a `r#Core` package. See
#4908.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Also makes the style guide explicitly comment on void, but this was the
intent IIRC because it matches Carbon's `-> ()` (and "always" versus
"except for void", which we definitely went back and forth on).
Includes adjusting function pointers, which I definitely forget this
syntax works sometimes.
Excludes utils/tree_sitter/src/scanner.c because it claims to be C, but
really we should probably fix that to be cpp.
At present, we typically define a DiagnosticConverter, then store an
instance of it and a DiagnosticEmitter that wraps it. This is relatively
minor in general, but I've been trying to create more self-contained
DiagnosticEmitter classes (which hold their own DiagnosticConverter,
similar to NullDiagnosticEmitter), and there it just gets in the way.
Since we don't reuse DiagnosticConverter instances, this combines the
definition into DiagnosticEmitter. Mainly this means we don't have a
separate object in play, and less to carry around.
The most impact is probably to SemIRDiagnosticConverter, which was also
the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some
different construction flow. Note in the PR I've split the file rename
to its own commit, to try to help delta views. However, the most
substantial parts of the refactoring are split into #4876, which this
depends upon.
I'm looking at eliminating `DiagnosticConverter`. This change removes
`NodeLocConverter` (albeit adding `UnitAndImportsDiagnosticConverter`),
and in doing so, refactors lex conversion functions to extract them out
from the `DiagnosticConverter` functions.
I'll be following up with changes that collapse `DiagnosticConverter`
logic into `DiagnosticEmitter` locations. The intent is that we
shouldn't need separate ownership of both types.
This switches most error printing to use diagnostics instead of direct
stream writes, even when not a specific file diagnostic. I'm allowing
empty filenames for this use-case.
This allows a little more specific testing to validate coverage of
output using the diagnostic coverage test. I'm adding a few tests to
cover things that weren't previously tested.
Separately, this also forces a little more standardization in format...
considering how changes like #4568 show effort being spent to _mirror_
diagnostic style, my thought is now to just use diagnostic code where
possible.
Note this also allows incrementally better testing of the language
server; I'm changing the crash fix from #4847 in favor of diagnostic
testing.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
- Escape dumped token strings (what got me here)
- Change the quoting from backticks to quotes
- Also add a `FormatEscaped` helper function for this, updating other
`.write_escaped` uses
Switching from RE2 to StrReplaceAll because it seems a fair fit for what
actually needs to be done here. Also pick up \t for visibility reasons.
This came up because clangd's LSP-related APIs print carriage returns.
This is a followup from #4834, I searched for "invalid" uses in our
codebase. This is mostly changing comments, and a couple debug
functions, but shouldn't affect testable behavior.
Note a couple things I'll highlight as not changing (but could) are:
- `ReturnTypeInfo::is_valid`
- `"invalid"` uses in the formatter
- `AddInvalid` for `!has_value` in `inst_fingerprinter` (because the
cases it's called sound invalid-ish)
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
This adds a RawStringOstream. Versus TestRawOstream, which is
consolidated over to RawStringOstream, it uses a string for storage
instead of a vector, mainly to support move-to-string semantics. Versus
llvm::raw_string_ostream, it owns the string and supports pwrite (which
is needed for driver and its fd_ostream compatibility requirement).
This converts most uses of llvm::raw_string_ostream, leaving behind a
few in InstNamer that explicitly cannot own the string, such as:
```
llvm::raw_string_ostream(name)
<< "_" << tree.tokens().GetColumnNumber(token);
```
I have this as its own library so that it can use CHECK.
Yes this doesn't save much code, but it's code we repeatedly write.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This removes some churn when adding new diagnostic cases to test files
(where previous to this change the newly added newline would cause the
previous diagnostic CHECKs to be updated including changes to the line
number because the CHECK for the blank line meant an extra line between
CHECK and source line).
A few alternatives discussed here:
https://discord.com/channels/655572317891461132/655578254970716160/1329573358475673723
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.
The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.
In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.
I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.
A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.
Add `EXTRA-ARGS:` support to file_test, to add arguments without
overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.
This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but this seems to be a generally useful feature anyway. Most
existing
tests using `ARGS` have been switched over to using `EXTRA-ARGS`.
Requested in review of #4716.
Pursuant to discussion regarding #4699, turn on
`misc-non-private-member-variables-in-classes` using the
`IgnoreClassesWithAllMemberVariablesBeingPublic` flag (the check treats
structs as classes, so we need this for structs with all-public
members). Updates the style guide notes to match, which should be pretty
minor due to the scoping of test fixtures.
Also fixes some underscore uses in test files on the way. Basically this
is keeping the style for [class data member
naming](https://google.github.io/styleguide/cppguide.html#Variable_Names)
even while making them public.
Disables three new warnings because they lean more towards style
conflicts than fixes. I've brought these up on #style.
Other than that, mostly fixing basic issues, and things that
clang-tidy-20 seems to fire where clang-tiday-16 didn't. One particular
curious case is `llvm::StringLiteral::data()` uses, which are flagged as
not strictly null-terminated; I'm switching to `const char*` in those
spots which matches `llvm::formatv`'s format argument, but feels worse.
I'm removing `run_clang_tidy.py` here because I'm observing it give
fewer warnings than `bazel build --config=clang-tidy -k
//toolchain/...`. The latter matches how we enforce in GitHub actions
(and also caches results, and suppresses output for files that have no
issues), so I'm dropping the bespoke script.
Just a small packing optimization. We currently have 222 `NodeKinds`, so
this reduces us to just 30ish more we can add without needing to pack
more. However, if we did, there would be a couple options for bringing
the count down by reusing `NodeKinds` and disambiguating based on the
token kind (the 29 infix operators as an example). Or we could just undo
this.
I'm expecting this to yield a small improvement. I'll see if I can get
better numbers since my machine's not really reliable, but here are some
basic values.
Also suggesting to draw the use of `::RawEnumType` for `TokenKind`,
since bit packing appears to work without it. Hoping the `static_assert`
is easier for people to understand the size of the field.
With the change:
```
----------------------------------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations Bytes Lines Tokens
----------------------------------------------------------------------------------------------------------------------------
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 50399 ns 50359 ns 14336 104.588M/s 3.87217M/s 21.8629M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 237823 ns 237629 ns 3072 136.721M/s 4.11986M/s 24.2058M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 997645 ns 996771 ns 768 142.343M/s 4.04105M/s 23.9363M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 4020308 ns 4018319 ns 192 152.041M/s 4.05966M/s 24.0874M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 16691390 ns 16683058 ns 48 151.317M/s 3.92374M/s 23.2936M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 75265735 ns 75233476 ns 8 135.842M/s 3.48421M/s 20.6862M/s
```
Without the change:
```
----------------------------------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations Bytes Lines Tokens
----------------------------------------------------------------------------------------------------------------------------
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 51515 ns 51480 ns 13312 102.312M/s 3.78789M/s 21.387M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 241040 ns 240900 ns 3072 134.865M/s 4.06392M/s 23.8771M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 985593 ns 984657 ns 768 144.094M/s 4.09077M/s 24.2308M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 4109327 ns 4105496 ns 192 148.813M/s 3.97345M/s 23.576M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 17459655 ns 17446006 ns 48 144.7M/s 3.75215M/s 22.275M/s
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 80802815 ns 80737489 ns 8 126.581M/s 3.24668M/s 19.276M/s
```
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
- Provide `Check::Dump(context, arg)` and similar.
- gdb and lldb should do contextual lookup, and `call Dump(*this,
Lex::TokenIndex::Invalid)` has been tested with gdb.
- Since this is only for debug, keeps the functions fully separated from
code.
- Uses alwayslink to ensure objects are correctly linked, even though
there are no calls.
- `-Wno-missing-prototypes` is needed when we don't have forward
declarations.
- Code is not linked in opt builds, using `#ifndef NDEBUG`.
- This probably could be doing something in BUILD files with a
`select()`, but the `#ifndef` seemed easier.
This is based on #4620, but uses free functions instead of member
functions.
Co-authored-by: Dana Jansens <danakj@orodu.net>
---------
Co-authored-by: danakj <danakj@orodu.net>
This is essentially the result of looking at `.begin()` uses. We also
frequently do `std::shuffle`, but unfortunately STLExtras doesn't
provide a wrapper for that.
This removes a lot of boilerplate `Print` functions in favor of a
CRTP-based approach that uses a `Label` field as an automatic prefix.
This `Label` is also made available for other purposes, particularly
`IdKind` crash messages in this change. In particular, for
`RequireIdKind` in node_stack.h from using numeric IdKinds (e.g., 5 and
24) to something that will print `IdKind(<label>)` (this came up
recently on #toolchain).
While I'm in here, also doing some other tinkering:
- Moving operators to be `friend` members, to reduce the extra
templating now that the base types are templated.
- Adjusts IntId diagnostics from `int [...]` to `int(...)` for
consistency with other id printing.
- Changes InstBlockId's label from "block" to "inst_block", since we
have multiple blocks now.
- Fixes StructTypeFieldsId to use "struct_type_fields" instead of
"type_block" (from `TypeBlockId`)
- Does some more adjustments from camelCase to snake_case for
consistency
I'm working to make sure remaining diagnostics have coverage, at least
the ones I'd previously added a TODO for. Note in particular that I
couldn't figure out a repro for UnaryOperatorRequiresWhitespace; if you
have one, I can add a test, but otherwise maybe it's actually
unreachable due to being diagnosed through infix logic (or, maybe
this'll let fuzzing tell me an example).
The first change here is to canonicalize away bit width when tracking
integers in our shared value store. This lets us have a more definitive
model of "what is the mathematical value". It also frees us to use more
efficient bit widths when available, such as bits inside the ID itself.
For canonicalizing, we try to minimize the width adjustments and
maximize the use of the SSO in APInt, and so we never shrink belowe
64-bits and grow in multiples of the word bit width in the
implementation. We also canonicalize to the signed 2s compliment
representation so we can represent negative numbers in an intuitive way.
The canonicalizing requires getting the bit width out of the type and
adjusting to it within the toolchain when doing any kind of math, and
this PR updates various places to do that, as well as adding some
convenience APIs to assist.
Then we take advantage of the canonical form and embed small integers
into the ID itself rather than allocating storage for them and
referencing them with an index. This is especially helpful for the
pervasive small integers such as the sizes of types, arrays, etc. Those
no longer require indirection at all. Various short-cut APIs to take
advantage of this have also been added.
This PR improves lexing by about 5% when there are lots of `i32` types.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Previously Collect() was used for types that implemented
CollectMemUsage() but otherwise Add() was used. This required the caller
to think about the type of the field and know/decide which method to
use.
Now, the caller always uses Collect() unless they are adding specific
byte values, in which case Add is used. Typically then, Add will only be
used to implement the CollectMemUsage() function.
To do this we require all Collect() methods to be templates so that they
all be a single overload set. The Collect on BumpPtrAllocator is
converted to a template that checks
`std::same_as<llvm::BumpPtrAllocator, T>`.
This is in anticipation of making the integer value store be customized
heavily. I'd like to extract it from the common code when doing that, so
first disentangling them here without any intended change in
functionality or behavior to enable that.
I've tried to update `#include`s to be as minimal as I can and added a
few missing includes spotted in the process.
I've split the test for value store to include what was easy focused on
just the value store templates rather than the unified shared value
stores.
This might surface some opportunities for adding more tests, but for
this PR, just doing the minimal restructuring.
This rejects type literals with more digits than we can lex without
APInt's help, and using a custom diagnostic. This is a pretty arbitrary
implementation limit, I'm wide open to even more strict rules here.
Despite no special casing and a very simplistic approach, by not using
APInt this completely eliminates the lexing overhead for `i32` in the
generated compilation benchmark where that specific type literal is very
common. We see a 10% improvement in lexing there:
```
BM_CompileAPIFileDenseDecls<Phase::Lex>/256 39.0µs ± 4% 34.8µs ± 2% -10.86% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Lex>/1024 180µs ± 1% 158µs ± 2% -12.22% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Lex>/4096 731µs ± 2% 641µs ± 1% -12.31% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Lex>/16384 3.20ms ± 2% 2.86ms ± 2% -10.47% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Lex>/65536 13.8ms ± 1% 12.4ms ± 2% -9.78% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Lex>/262144 64.0ms ± 2% 58.4ms ± 2% -8.70% (p=0.000 n=19+18)
```
This starts to fix a TODO in the diagnostic for these by giving a
reasonably good diagnostic about a very large type literal. However, in
practice it regresses the diagnostics because error tokens produce noisy
extraneous diagnostics from parse and check currently. Leaving the TODO
there, and I have a follow-up PR to start improving the extraneous
diagnostics.
Trying to make split file tests of lex functionality shorter and easier
to read. numeric_literals.carbon in particular has an example of why I'm
interested in this (at the bottom). This also switches from `[]` list
format to `-` list format so that the trailing `]` is removed.
Trimming comments in tokenized_buffer.h because (1) it feels like it's
giving too much detail about what's printed, which has drifted slightly
and (2) it also feels like it's trying to justify YAML output, when
that's just what we're doing in general.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Also surround it in square brackets rather than parentheses. This
matches the format used by Clang and GCC, and means diagnostics will
still match the `file:line:col: error: ` pattern used by some IDE tools.
Before:
```console
fail_builtins.carbon:11:11: error(AliasRequiresNameRef): alias initializer must be a name reference
```
After:
```console
fail_builtins.carbon:11:11: error: alias initializer must be a name reference [AliasRequiresNameRef]
```
Also tighten up test regex to only match on `STDERR` lines that list a
file name.
Previously in an optimized build we'd produce bogus tokens, such as
tokens with incorrect IdentifierIds, and in a debug build we would try
to CHECK-fail -- but actually wouldn't, because we're incorrectly
checking for `2 << bits` instead of `1 << bits`. I hit this while I was
trying to do some profiling and was seeing some very strange
diagnostics.
The diagnostic is pointed at the first token that is beyond the limit to
help people determine where to split their files.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Use the diagnostic kind printing in #4425 to catch when we have
diagnostics with no tests.
This merges a couple other use-cases of filegroup manifests into a
common rule.
Note I do add a few tests for things, and also some things are
_actually_ unit tested (just not in the file_test structure). But I
stopped when I realized that dealing with merge conflicts is going to be
a pain. I might end up reverting test changes (as part of merge conflict
resolution) and doing narrow test additions in a separate PR, after both
this and #4425 are merged.
Building on #4411, avoid using StringLiteral in format strings. This
includes a diagnostic check to prevent regressions (which is also how I
gathered issues).
Note, I haven't looked at `std::string` uses yet, but we might need
things like that to be able to pass strings in code back to the user.
StringLiteral though means that it's literally written down in the
toolchain, at which point it should probably be written in the format
string instead of separately.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This is to help identify which diagnostics we're actually using.
Note that driver/testdata still has tests which don't pass this flag,
and so continue to test the kind-less (default) behavior.
Building on https://github.com/carbon-language/carbon-lang/pull/4411,
replace format_provider uses (other than `TokenKind`, which is more on
the okay side of things)
Also does some edits to `ClassMemberDefinition` to try to better match
diagnostic style
This is improving the comment production to produce fewer distinct
comments.
At present, comment processing uses strict prefix matching. It either
expects `// ` (with a space) for valid comments, or just `//` (without a
space) for invalid comments that lacked the space.
As a consequence, the following would be three comments:
```
// Comment 1
//
//
// Comment 4
```
This is because a 3-character prefix is used for valid comments. The
prefix switches between lines 1 and 2, and again between lines 3 and 4,
each resulting in a separate comment.
For contrast, this is one comment because only a 2-character prefix is
used:
```
//Comment 1
//
//
//Comment 4
```
That's because all lines lack a suffix space.
Additionally, with SIMD 16-byte boundaries, further splits can occur if
processing needs to transition to non-SIMD.
Here, I'm trying to just address all of this by:
1. Stitching together adjacent comments. Since a lexed comment starts at
the `//` excluding the indent, the delta from the prior comment must be
precisely the indent.
2. Adding support for switching from SIMD to non-SIMD on file
boundaries.
I considered trying to have a separate `//\n` prefix for SIMD processing
of `// `, but I wasn't sure about the tradeoff of doing both at the same
time (in particular, it'd require constructing a string for the
different prefix), thus this stitch approach. This does mean multiple
passes will be required for a typical long comment structure using blank
comment lines to separate paragraphs (for performance reasons, I will
recommend engineers not write comm... nevermind).
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Note in particular that this fixes an issue where SharedValueStore had
been shared across files, when they should be per-file. This is only
visible when doing multiple compilations in a single test, which was
rare before.
This also moves these tests into the Testing namespace. My memory of the
various namespacing changes is that we'd generally agreed to have tests
in Testing so that we'd see SemIR:: and similar, same as we would in a
lot of the implementation.
Thomas Köppe
Jon Ross-Perkins
Boaz Brickner
David BlaikieandJon Ross-Perkins
Dana Jansens