[PATCH] D120495: [clang][dataflow] Add transfer functions for structured bindings

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked 2 inline comments as done.
sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:80
+///
+/// FIXME: Consider adding support for structured bindings to the CFG builder.
+class DecompositionVisitor : public ConstStmtVisitor<DecompositionVisitor> {
----------------
xazax.hun wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > Did you look into how hard would it be to add structured bindings to the CFG builder? If the effort is comparable to this patch (and not significantly bigger), it might be better to do that work instead of spending effort on some temporary workaround. What do you think?
> > Circling back to this after a while. I believe we explored changing the CFG briefly, but don't have a fully fleshed out proposal for it. I recently noticed https://discourse.llvm.org/t/implement-support-for-c-17-structured-bindings-in-the-clang-static-analyzer/60588. It seems that part of the project is exploring necessary changes to the CFG. What do you think about submitting this patch with local pattern matching and revisiting that once the GSoC project completes?
> Officially, there is no candidate accepted for that project yet, so there is no guarantee that we will have someone working on this. That being said, the CSA devs started to discuss what would be the best way to represent structured bindings in the CFG, and it is possible that for many of the cases we do not actually need to change the CFG, because the AST nodes have rich information. But of course, none of this is final, but we do not need to block this patch on that. So I'm fine adding this for now.
> 
> The AST for the supported case looks like:
> ```
>    `-DeclStmt 0x5599ad9de6b0 <line:8:7, col:45>
>       `-DecompositionDecl 0x5599ad9de310 <col:7, col:42> col:13 used 'A &' cinit
>         |-DeclRefExpr 0x5599ad9de388 <col:42> 'A' lvalue Var 0x5599ad9ddb80 'Baz' 'A'
>         |-BindingDecl 0x5599ad9de280 <col:14> col:14 BoundFooRef 'int'
>         | `-MemberExpr 0x5599ad9de630 <col:14> 'int' lvalue .Foo 0x5599ad9dd970
>         |   `-DeclRefExpr 0x5599ad9de610 <col:14> 'A':'A' lvalue Decomposition 0x5599ad9de310 '' 'A &'
>         `-BindingDecl 0x5599ad9de2c8 <col:27> col:27 BoundBarRef 'int'
>           `-MemberExpr 0x5599ad9de680 <col:27> 'int' lvalue .Bar 0x5599ad9dd9d8
>             `-DeclRefExpr 0x5599ad9de660 <col:27> 'A':'A' lvalue Decomposition 0x5599ad9de310 '' 'A &'
> ``` 
> 
> So each `BindingDecl` is like:
> ```
>         `-BindingDecl 0x5599ad9de2c8 <col:27> col:27 BoundBarRef 'int'
>           `-MemberExpr 0x5599ad9de680 <col:27> 'int' lvalue .Bar 0x5599ad9dd9d8
>             `-DeclRefExpr 0x5599ad9de660 <col:27> 'A':'A' lvalue Decomposition 0x5599ad9de310 '' 'A &'
> ```
> 
> Is there a case where the `BindingDecl` would have a different shape? If we do not expect that happening, I feel like an `StmtVisitor` is a bit of an overkill for this and we could handle the supported case more directly. What do you think?
Agreed. I simplified the patch. Please take a look.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120495/new/

https://reviews.llvm.org/D120495