This replaces GetAstMatcher with AddMatcher because cxxForRangeStmt is a StatementMatcher. addMatcher has multiple definitions (https://clang.llvm.org/doxygen/classclang_1_1ast__matchers_1_1MatchFinder.html) and so this approach allows using the right addMatcher without writing per-call overloads.
To handle the `var`, I'm considering something like moving VarDecl logic into a VarMatcherBase so that I can just use CXXForRangeStmt's getLoopVariable. The problem is a for-range statement has multiple VarDecls, and getLoopVariable may be the easiest way to identify the real one.
We will eventually need the ability to materialize temporaries eventually, but we don't really seem to need it yet, and when we do, I think we should do it in a way that gives uniform treatment to temporaries and local variables.
Also:
- Switch data members, and associated parameters and return types to be values/references rather than pointers, where applicable.
- Move Expression's TagVisitor to anonymous namespace, to avoid name collision.
- Switch to trailing return syntax (per style guide).
Propose the decision from #542, noting implementation from #563
Also integrates some of #339 into `variables.md` because that's actually how this started, looking for a proposal reference for #542
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Pretty much any names will be awkward, since these types represent cases where an `Expression` is not an expression, and a `Value` is not a value, but we can at least be more explicit about the fact that they represent bindings rather than variable usages.
- `Kind` enumerator names always match the corresponding factory function, accessor, and type names (if any).
- All abbreviations in those names are expanded.
- Those names always have a suffix to disambiguate expressions from values.
`VarTV` is excluded from these changes because there's a pending PR to remove it.
This ensures that the factory functions correspond 1:1 with expression kinds, and MakeOp covers their use cases with minimal syntactic overhead now that it can take initializer list arguments.
* moved most of logic from HandleValue into the Step functions
* removed the HandleValue function
* added some comments
* responses to Geoffrey
* move increment of act->pos out of the step logic for value actions
* changed act->pos to start at 0 and not have any gaps
* Update executable_semantics/interpreter/action.h
Co-authored-by: Geoff Romer <gromer@google.com>
* update comment based on Geoffrey's suggestion
Co-authored-by: Geoff Romer <gromer@google.com>
The built-in GitHub action cache is too broken for us to use currently.
Pull request runs will evict trunk runs from the cache easily, which
will remove a useful baseline and make builds generally uncached and
extremely slow.
Instead, move to using Bazel's remote caching functionality with a GCP
storage bucket. This gives us nearly limitless storage and is actually
vastly simpler than the prior arrangement. It should also allow cache
sharing between different build configurations and other benefits.
This cache isn't usable by anyone else sadly, but it should at least
provide a better scaling technique for our CI.
This does require us to move from `pull_request` workflow to
a `pull_request_target` workflow and run the CI within the domain of the
project. This is only really safe for us to do while the project remains
private. When we go public we'll want to make the cache publicly
readable and change this code so that when not running on a branch in
the main repository we use the cache in a read-only mode and only write
back updates for branch runs. This should still be effective but
requires a bit more complexity here and in the remote cache including
setting up public access. For now, going with the simple if slightly
less secure model.
Range-based for loops remain messed up -- I'm just disabling the test here and turning a blind eye.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This avoids having the stacked PR get auto-closed in step 7, and clarifies that you need to merge from trunk to the formerly-stacked PR in order to avoid spurious diffs.
TODOs in the test for known issues.
I may switch the approach to getting the variable type (on examination, this isn't working quite as well as I'd thought) but for now I think it's okay. I had an earlier approach though that may work better overall -- I'd been thinking this would work better, but as you can see in the null check for type information, I think I missed a key point.
Anyways, what'd really been vexing me was `int i, j` which I think I handle passably well now. There's obviously room for improvement, but given I've been going at this for a couple days now, I thought it best to checkpoint where I was.
This also includes some related framework changes to fix bumps I was running into. Overall the tool should operate a bit more smoothly with these changes. There are still issues with overlapping replacements, but I think it's primarily with range-based for loops which I just need to take some time to fix.
This was necessary when I was testing this on my own PR branch, but
without this rebuilds on trunk won't save a fresh cache for folks to use
on *their* pull commit branches. We just need a fresh key here.
Also add one more flag to our `.bazelrc` to improve the overall build
efficiency.
While the first build with this will be very slow (likely a couple of
hours), the caching layer we are using now should effectively cache this
for subsequent builds so that only the changed files have to be
recompiled. Updates to Clang and LLVM themselves will still result in
a slow rebuild, but normal development should remain fast.
The presence or absence of whitespace is used to determine which
operator is in use, following the rules described in #520.
Support for prefix * dereference operator follows #523.
Co-authored-by: Geoff Romer <gromer@google.com>
Geoff Romer
Jon Meow
josh11b
Richard SmithandChandler Carruth
Jeremy G. SiekandGeoff Romer