This creates a new check/type.h for most logic, and also moves some
functions to TypeStore in sem_ir/type.h. My approach for TypeStore is to
focus on moving the read-only functions there.
`ids.h` and `ids.cpp` are the manual edits, everything else is
search-and-replace.
The full list of things moved is:
- `TypeId::TypeType`
- `TypeId::AutoType`
- `TypeId::Error`
- `ConstantId::Error`
This is to unblock removing `InstId::Builtin*`.
This is for more clearly distinct names, and to make it a clearer
transition from `BuiltinInst` for name conflicts. `FloatType` is also an
instruction, and we have `Carbon::Error` (common/error.h). This avoids
affecting tests, although the name is embedded in the builtin test.
In `LegacyFloatType`, `Legacy` because I was having trouble coming up
with a more appropriate name. I'm not clear this is a `FloatLiteralType`
at present, it needs some work to mirror `IntLiteralType`.
In `ErrorInst`, the suffix `Inst` was discussed as good and similar to
`BuiltinInst` (although I'm trying to get rid of that).
Instead of leaving array bounds as whatever integer type they arrive as,
convert them to the `IntLiteral` type as part of forming an `ArrayType`.
This ensures that array types canonicalize properly even when the bounds
are specified with different types.
Create an empty generic definition for a generic builtin function to
avoid this causing "use of undefined generic function" errors.
This is a primarily automated change:
- Search & replace for capitalization
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s")([A-Z])`
- `$1\L$2`
- Search & replace for period
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s"(?:[^)]|\n)+)\.("[,)])`
- `$1$2`
- Limited search & replace for `ERROR: ` -> `error: ` in streamed things
- Leaving a TODO for command_line because there's more cleanup that can
be done there
- Modify diagnostic_consumer.cpp
- ERROR -> error
- WARNING -> warning
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This is for consistency with #4120. Similar to that, we can use
overloads on the typed NodeId rather than individually named handlers.
There isn't the same caller benefit here though, since the calls from
check.cpp are already boilerplate.
Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.
Most other cases are functions that should have been marked `static` but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.
One complicating factor was that we weren't including the `handle.h` for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be `static`
and which don't. For the `parse` library I had to add the `handle.h`
header as well, I tried to match the design of it in `check`.
I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: https://github.com/llvm/llvm-project/issues/94138
I also had to use some hacks to work around limitations of Bazel rules
that wrap `cc_library` rules and don't expose `copts`. I filed a bug for
`cc_proto_library` specifically:
~https://github.com/bazelbuild/bazel/issues/22610~https://github.com/bazelbuild/bazel/issues/4446
Trying to conform with #4009. Changes SemIR::LocIdAndInst construction
to root out struct init cases with AddInst and related functions. I'm
using templating of AddInst functions in order to avoid `AddInst(loc_id,
InstName{...})` and instead have `AddInst<InstName>(loc_id, {...})` with
I think similar readability results. There are a couple cases where inst
construction is templated and so designated initializers couldn't be
used, so this may be better for those in particular due to the extra
type enforcement.
This probably doesn't clean up every last case, but I was trying to get
the bulk at once without bleeding over into less related changes.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This was previously discussed at
https://discord.com/channels/655572317891461132/655578254970716160/1209975051588210729.
I'm initiating this mainly because we typically use "id" suffixes to
indicate an `IdBase` being passed around and the non-id suffix of
`parse_node` suggests at it carrying more data than it actually does.
There used to be more reason for avoiding `node_id` because
`SemIR::InstId` used to be named `NodeId`, but that's no longer
necessary. As a consequence, I'd like to rename `parse_node` to more
precisely reflect its type.
In full, this is doing:
```
parse_node_kind -> node_kind
parse_node -> node_id
ParseNodeCategory -> NodeCategory
ParseNodeKind -> NodeKind
ParseNode -> NodeId
```
This is primarily in check and sem_ir, but with some `parse_node_kind`
references in parse too.
Pluralization is consistent with name forms on both sides, so that
wasn't part of my replacements.
This required adding a few headers that were found transitively before,
but not too many. This is sadly a fairly manual process of opening every
file in my IDE, but I think I got everything in `//common` and
`//toolchain`.
There are a few cases where technically we don't need `foo.h` to be
included into `foo.cpp`, but I've forced those to stay with a pragma.
I've tried to catch the places where we can cut deps in Bazel as well,
but not sure I got all of those.
I had been noticing these in other PRs and it seemed better to isolate
the change.
Also, form `addr_of error` instead of `addr_of operand` when `operand`
is not a reference expression, so that we don't try to constant-evaluate
a meaningless expression.
Also, expect a constant for an array bound rather than specifically an
integer literal. This allows constant evaluation results to be more
easily tested by inspecting array bounds.
The parse nodes are still tracked as part of the same value store
interface in order to ensure parity, but they're split out from Inst
itself in order to reduce the size of Inst -- the expectation is that
they don't need to be passed around quite as much.
This change doesn't actually reduce the passing very much, although
there are hints of it: AddInstAndPush doesn't typically need a separate
parse node from the one on the Inst itself, for example. In a couple
spots I changed code to rely a little more on the InstId until the
ParseNode is needed, but it's very low hanging fruit where done. I think
convert could do more to not eagerly fetch the parse node before its
use, but more cleanup felt it would be easier to handle separately. I'm
currently viewing this as making such cleanup _possible_ rather than
executing on it up-front.
But also, I want to make sure there's a consensus to head in this
direction before pulling the trigger. We speculated that this would
result in the parse node being passed around less, and I do think that's
the case, although it's a bit fuzzy in the change.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Goal is to increase type safety, though more work needs to be done (see
added TODOs).
Note that, after this change, check handlers corresponding to deleted
parse node kinds will no longer compile.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
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.
These are manual fixes; mostly from clang-tidy, some from clangd (which
notes unused includes).
In typed_insts, adding inlline due to misc-definitions-in-headers. Per
discussion, clang-tidy is wrong, but inline silences it.
For parameter name skew in definition versus declaration, I'm just using
the name from the definition.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
1. In general, `semantics_ir` -> `sem_ir`, to match the directory name.
2. For the list of `ValueStore`-related accessors on `SemIR::File`, add
them to `check`'s `Context` object, shortening access.
Finishing what #3316 started, add more bespoke ValueStore-like
structures to File. With this, the things which previously had somewhat
boilerplate Add/Get functions are now all on side classes, giving a
uniform style of API for calling.
Note, I was on the fence about making things public on ValueStore. If
it's preferred that I make some things there protected I certainly can,
there's just a trade-off that may mean more distinct child/wrapper
types.
Split `node.h` into separate files for ID types (`id.h`) and for typed
nodes (`typed_nodes.h`). The per-node-kind data is now specified as part
of declaring the typed nodes, and is removed from the node kinds
x-macros, which now simply enumerate the node kinds.
Building on #3311, change SemIR to use the SharedValueStore. Since this
removes hermeticity, raw output no longer prints ints, reals, and
strings. TokenizedBuffer accessors are modified to return IDs because
values are often passed through in semantics without needing to read
them.
I would've put SharedValueStores on Context, except for the
GetArrayBoundValue convenience method. I felt awkward removing that, so
it's on File, at least for now. That's then used by the formatter and
Lower too. The flipside of this is that TokenizedBuffer has a
SharedValueStores only for printing, so maybe that's similar enough to
what File is doing.
This doesn't start shifting other SemIR members to ValueStore, but that
seems like a next step.
Replace `SemIR::Node::GetAsFoo` and `SemIR::Node::Foo::Make` with
`SemIR::Foo` class that represents a particular kind of node, with named
fields.
Rename `SemIR::IntegerLiteral` and `SemIR::RealLiteral` to
`IntegerValue` / `RealValue` to better reflect their purpose and avoid a
name collision with the corresponding `SemIR` node kinds.
Remove `NodeKind::Invalid` and the `SemIR::Node` default constructor
entirely, as they were not used for anything.
Combine the initialization, implicit conversion, and value category
conversion functions into a single function.
This substantially reduces the duplication between these steps, and
ensures that we support the same set of conversions in all these
contexts. This also fixes some issues where we would not use the proper
value representation for tuples and structs after performing implicit
conversions.
In general, LLVM's parsing of decimal integers to `APInt`s forms an
`APInt` that is 4n bits wide, where n is the length of the integer, and
the `isNegative` check only checks the high bit.
In this case, we form an `APInt` that is four bits wide, with the high
bit set, which we reject because we think it's "negative". These two
array lengths are the only ones where this happens -- if the decimal
integer value is two characters long, we form an `APInt` that is eight
bits wide but holds a value < 100, so the high bit is never set, and the
same applies for longer integers too.
An `IntegerLiteral` is never negative, so we don't need the `isNegative`
check, and in fact it only detects the n=8 and n=9 cases.
Continuing with #3070. Just a dir and file rename (mostly removing
prefixes, although for parse_tree_fuzzer and parse_tree_file_test I'm
dropping "tree" instead of "parse"). Everything in the parse dir should
be marked as a move.
Continuing along with #3070. Note this is just a file rename, with BUILD
edits; every file previously in semantics/ should show as moved (except
maybe BUILDs, which split).
Jon Ross-Perkins
Richard Smith
Chandler Carruth
josh11bandChandler Carruth
Geoff Romer