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 switches `DCHECK` and `FATAL` as well.
The goal is to reduce the code size impact of these assertions so that
we can keep more of them enabled. Currently, the largest cost I see from
`CHECK` is not the actual check or the cold code itself, but actually
the failure to inline trivial functions due to the presence of the cold
code. This means that our goal isn't to reduce apparent code size in the
final binary but the LLVM IR cost assessed for these routines in the
inliner, which closely correlates with code size but is a bit different.
As discussed in #4283, experimentation shows that a single function call
with a minimal number of arguments is the lowest cost model for these.
This is easily achieved with a format-string API that internally uses
`llvm::formatv`. This PR is essentially the `CHECK` version of #4283.
However, the check macros are substantially harder to make work with
both format strings and streaming because they also take a condition.
Also, unexpectedly, I was very successful at devising a regular
expression based automated rewrite from the streaming to the format
string form with only low 10s of manual fixes. This includes compacting
strings broken up across lines, etc. Given how well that went, I've
prepared this PR which just directly switches to the format string API
and migrate everything to use it.
One nice side-effect is that the format string approach ends up greatly
simplifying the implementation here as well.
This is ... *shockingly* effective. Parsing speeds up by more than 3%
with just this change. And checking speeds up by **8%** with this change
alone:
```
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 86.3µs ± 1% 82.9µs ± 1% -3.94% (p=0.000 n=17+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 431µs ± 1% 415µs ± 1% -3.76% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 1.77ms ± 1% 1.71ms ± 1% -3.18% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 7.44ms ± 1% 7.17ms ± 2% -3.56% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 30.7ms ± 1% 29.7ms ± 1% -3.15% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 131ms ± 1% 127ms ± 1% -2.81% (p=0.000 n=18+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/256 878µs ± 2% 800µs ± 1% -8.91% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/1024 1.88ms ± 2% 1.72ms ± 1% -8.56% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/4096 5.78ms ± 2% 5.28ms ± 1% -8.70% (p=0.000 n=20+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/16384 21.9ms ± 1% 20.1ms ± 1% -8.02% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/65536 90.4ms ± 2% 83.1ms ± 1% -8.04% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/262144 381ms ± 2% 352ms ± 1% -7.79% (p=0.000 n=19+19)
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Includes support for the `impls`, `=`, and `==` requirement operators to
the right of a `where`, but `and` to allow multiple requirements is
still a TODO.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Just a small factoring thing. The `if` logic is sizeable, and there's no
need for it to be in handle_expr (we do also have handle_brace_expr,
handle_index_expr, etc)
Applies #4278 TEST_NAME substitution to tests. Note I've tried to
structure commits as:
1. Do all the replacements.
2. autoupdate (nothing else) -- this shows incorrect updates.
3. Fix up manually, including autoupdates to get back to original
output.
These appear in `where` clauses, as in:
```
U:! InterfaceB where .C = Vector(.D)
V:! type where Vector(.Self) impls Sortable
```
`.Self` can additionally appear in type expressions in a binding pattern
such as `T:! InterfaceA(.Self)`.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Most of these are about enabling inlining, in a couple of cases moving
code to a header and throughout switching to `CARBON_DCHECK`. The code
size of `CARBON_CHECK` seems to make inliing quite unreliable. I'm going
to think about whether there are ways to improve this, but a reasonably
small number of these seem worth switching for now to get some compile
time savings.
Also moves VLOG out of the hot path which helps a bit as well.
All combined, this net a bit over 10%, although it varies a bit exactly
how much. We're now pretty consistently over 800k lines/second for check
in the compilation benchmark for files >=4k lines, which makes me happy.
That's remarkably close to our original target.
Not really planning to keep optimizing here, just was glancing at the
profile and many of these stood out to me and were easy to fix.
Move subtree sizes over to TreeAndSubtrees, using the different
structure to represent the additional parse work that occurs, as well as
making it clear which functions require the extra information. My intent
is to make it hard to use this by accident.
The subtree size is still tracked during Parse::Tree construction. I
think a lot of that can be cleaned up, although we use it during
placeholder assignment so it may take some work. I wanted to see what
people thought about this before taking action on such a change.
I'm using a 1m line source file generated by #4124 for testing. Command
is `time bazel-bin/toolchain/install/prefix_root/bin/carbon compile
--phase=check --dump-mem-usage ~/tmp/data.carbon`
At head, what I'm seeing is:
```
...
parse_tree_.node_impls_:
used_bytes: 61516116
reserved_bytes: 61516116
...
Total:
used_bytes: 447814230
reserved_bytes: 551663894
...
1.43s user 0.14s system 99% cpu 1.565 total
```
With `Tree::Verify` disabled completely, it looks like:
```
parse_tree_.node_impls_:
used_bytes: 41010744
reserved_bytes: 41010744
...
Total:
used_bytes: 427308858
reserved_bytes: 531158522
...
1.20s user 0.13s system 99% cpu 1.332 total
```
Re-enabling just the basic verification (what is now `Tree::Verify`),
I'm seeing maybe 0.05s slower, but that's within noise for my system. I
do see variability in my timing results, and overall I think this is a
0.2s +/- 0.1s improvement versus the earlier (always testing `Extract`
code) implementation. That's opt; debug builds will be unaffected,
because the same checking occurs as before.
Note, the subtree size is a third of the node representation, which is
why I'm showing the decrease in memory usage here.
Allow directly constructing a PostorderIterator, to get rid of
`tree.postorder(node_id).end()` indirect construction. For ranges that
don't need tree data, make it clearer that they're not validated.
Note, this subtly gets rid of a subtree size use in the
`tree.postorder(node_id).end()` case (to get the discarded `begin()`
value).
As discussed in toolchain meeting, we want to avoid overloading the
meaning of "instance", and "specific" was the best name we found. It's a
little unorthodox and inventive, but hopefully over time will become as
unsurprising as the term "generic" is.
Note this isn't implementing checking through imports. The parse node
there is harder to access through the context, so would require
examining the entity in order to get the import declaration, to get at
the ImportIR. We also don't have a parse tree attached in that case, and
would need to add one to SemIR::File. But I believe we do want to add
that, so it's explicitly a TODO.
Note GetTokenText re-lexes literal values, so there's a bit of potential
overhead there. Not sure if we want a more efficient manner for
comparing in cases like this.
Also adds import_ir_scope to namespace formatting. I'd done this as an
aid for #4153, and am splitting it out.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
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
Following up on discussion from #3948, doing a general rename of
"enclosing scope" to "parent scope" (and "enclosing scopes" to "ancestor
scopes"). The intent is to improve understandability and collide less
with C++ terminology for "enclosing scope". Note this changes most uses
of "enclosing", but leaves behind a few like "enclosing function" and
"enclosing block".
Note this does create some "parent class" mentions for "adapt" and "var"
(the class they're within), which is maybe unfortunate, but we'd
probably say "base class" if we meant inheritance so perhaps that's
okay. Along the same lines, these are the only `parent_class` uses I see
now, and we do have a few `base_class`.
This has is a nice-to-have for me. Frequently I want to run a specific
test, and end up digging through output to be able to copy-paste the run
line. This uses TIP lines to inject the command into the file when using
AUTOUPDATE.
Note, one of the reasons I want this is because "bazel test
//toolchain/testing:file_test --test_output=all" has been regularly
exceeding bazel's output limit for me (workaround is either opening the
output file or specifying an obscure output limit flag), making it a
little harder for me to get the commands. However, frequently I'm adding
a file and want to iterate on it, so that's really the use case I have
in mind here.
Instead of adding a `Token<Kind>` field to typed parse nodes that
contains a `TokenIndex`, make the parse node directly hold a
`TokenIndex` of a more specific kind.
Just spotted these while looking at warnings that seem to fire on our
code are probably are things we'd fix if we saw them. None of these seem
important FWIW.
Also removes a redundant flag that is part of `-Wall`.
I have a follow-up for the high-value warning I spotted that motivated
me to look at all of this. But it's noisy so kept it as a separate PR.
Instead of tracking the token associated with a parse node in the `.def`
file macro, track it on the typed node instead. List the token as a
field inside the node structure to show the order of the token relative
to the other components of the grammar production, and to allow the
token index to be accessed when the node is extracted.
Remove the corresponding information from the `.def` file, leaving
behind just a list of parse node kinds in the majority of cases.
This also removes the checking of the token kind associated with a parse
node in the case where the parse node has errors. Previously we had a
flag on the node kind to indicate whether we should check this, but per
[discord
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1246214418979881052),
we have decided to remove this.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Mirroring #4003 for NodeCategory.
Note we template a lot more on NodeCategory's enum, so this is a
slightly more awkward delta.
Also, switch from Enum in KeywordModifierSet to RawEnumType for
consistency with EnumBase. The templating on NodeCategory had me
thinking about that more.
Instead of tracking the bracketing and child count information in the
kind macro in `node_kind.def`, provide it to `NodeKind::Define` in
`typed_nodes.h`. If a node is both bracketed and has a fixed child
count, track both facts and check them both in tree verification, since
it's easy to do so now.
The overall goal here is to reduce `node_kind.def` down to a simple list
of names. I have a slightly different approach in mind for the token
kinds.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Parse the name of a declaration as a sequence of `NameQualifier`s --
which have a name, possibly parameters, and a trailing period --
followed by a name and possibly parameters. This prepares us for parsing
declarations of members of generic classes and similar cases, but
actually supporting such member redeclarations is left to a future
change.
We previously required functions to have parameters, but no longer do,
following the direction of #3848. Cases like namespaces that can't
actually have parameters are now diagnosed in check instead of in parse.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I think the template files are simply an oversight. The fuzz files
should probably be updated because failing on the `package` line isn't
an interesting test of logic. Plus one minor comment edit.
This isn't *technically* a C-style cast, but a function-style cast. And
newer versions of `clang-tidy` no longer fire on this code, so it is a
little sad to have to work around. But the version on our build bots
seems old enough to still hit this issue (and possibly some others).
Our merge queue also doesn't seem to catch these reliably, but that's
for a separate PR.
Change syntax for package declaration to put the `impl` keyword at the
start and remove the `api` keyword.
To support this, rearrange processing of package, library, and import
declarations to use the general modifier handling support in declaration
parsing rather than special-case logic.
There is an ambiguity in `impl package.Foo as Bar`, which we resolve by
treating `package` as an introducer after a modifier only if it's not
followed by `.`.
This provides `export import` logic in lex, parse, and check; `export
name` logic is only in lex and parse, not check.
I think with `export name` I'm going to need to modify import_ref and
some consolidation logic, whereas `export import` seems feasible to keep
as primarily import logic. Given the implementations were looking like
they'd diverge more substantially, I thought it'd be helpful to cut the
PR here.
Jon Ross-PerkinsandRichard Smith
Chandler Carruth
josh11bandJosh L
David Blaikie
Brymer Meneses
