[PATCH] D115187: [clangd] Expose CoawaitExpr's operand in the AST

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall added a comment.
nridge updated this revision to Diff 407028.
nridge updated this revision to Diff 420107.
Herald added a project: All.
nridge retitled this revision from "[WIP] [clangd] Attempted fix for issue 939" to "[clangd] Expose CoawaitExpr's operand in the AST".
nridge edited the summary of this revision.
nridge published this revision for review.
nridge added reviewers: sammccall, kadircet.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.

FWIW I agree the "store" option would be more reliable here and also simpler - I think it's just storing one more pointer per co_await, and one we have easy access to.
(I would be worried about what happens to that expression in template instantiation, but dependent CoroutineSuspendExprs only store the written form anyway, so it probably actually becomes cleaner)

A change that probably goes along with this one is having RecursiveASTVisitor traverse the syntactic form instead of the semantic one if traversal of implicit code is off.


nridge added a comment.

Rework to store operand instead of digging it out from the common-expr


nridge added a comment.

(Keeping this in the WIP state for now, there's some crashiness I need to sort out)


nridge added a comment.

Add clang test


nridge added a comment.

Can't seem to reproduce the crashiness any more, so I wrote a clang test and I'm submitting this for review.

I'm happy to take guidance on what the clang test should look like. What I have currently is a FileCheck test to verify that a CoawaitExpr has the expected subexpressions. I added a new file for this because the existing coroutine-related tests that I could find did not use FileCheck (they were just testing for expected-errors and such).


Previously the Expr returned by getOperand() was actually the
subexpression common to the "ready", "suspend", and "resume"
expressions, which often isn't just the operand but e.g.
await_transform() called on the operand.

It's important for the AST to expose the operand as written
in the source for traversals and tools like clangd to work
correctly.

Fixes https://github.com/clangd/clangd/issues/939


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115187

Files:
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/co_await-ast.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115187.420107.patch
Type: text/x-patch
Size: 19622 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220404/059704b7/attachment-0001.bin>