[PATCH] D77809: [Analyzer] Include typedef statements in CFG build.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/test/Analysis/cfg.cpp:570
 }
 
+// CHECK-LABEL: void vla_simple(int x)
----------------
balazske wrote:
> aaron.ballman wrote:
> > NoQ wrote:
> > > balazske wrote:
> > > > balazske wrote:
> > > > > aaron.ballman wrote:
> > > > > > Can you also add tests for:
> > > > > > ```
> > > > > > int vla_unevaluated(int x) {
> > > > > >   // Evaluates the ++x
> > > > > >   sizeof(int[++x]);
> > > > > >   // Does not evaluate the ++x
> > > > > >   _Alignof(int[++x]);
> > > > > >   _Generic((int(*)[++x])0, default : 0);
> > > > > >   
> > > > > >   return x;
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > Also, it's a bit strange to find these VLA tests in a .cpp file. VLAs are a C++ extension and we should have some testing for them for constructs that are C++-specific, but I would have expected many of these tests to be in a C file instead.
> > > > > Could be there platforms (on buildbot?) where the VLA code (in cpp file) does not compile? Is it worth to have a test with VLA type alias then?
> > > > I have this output for the test above (without the new code), is it correct?
> > > > (The `++` seems to be evaluated for the `alignof` statement too.)
> > > > ```
> > > > int vla_unevaluated(int x)
> > > >  [B2 (ENTRY)]
> > > >    Succs (1): B1
> > > > 
> > > >  [B1]
> > > >    1: x
> > > >    2: ++[B1.1]
> > > >    3: [B1.2] (ImplicitCastExpr, LValueToRValue, int)
> > > >    4: sizeof(int [++x])
> > > >    5: x
> > > >    6: ++[B1.5]
> > > >    7: [B1.6] (ImplicitCastExpr, LValueToRValue, int)
> > > >    8: alignof(int [++x])
> > > >    9: 0
> > > >   10: x
> > > >   11: [B1.10] (ImplicitCastExpr, LValueToRValue, int)
> > > >   12: return [B1.11];
> > > >    Preds (1): B2
> > > >    Succs (1): B0
> > > > 
> > > >  [B0 (EXIT)]
> > > >    Preds (1): B1
> > > > 
> > > > ```
> > > > Could be there platforms (on buildbot?) where the VLA code (in cpp file) does not compile? Is it worth to have a test with VLA type alias then?
> > > 
> > > That's unlikely; the problems with VLAs in C++ are IIRC about the bureaucracy of the Standard (i.e., they couldn't solve enough cornercases, like whether `T<int[x]>` and `T<int[y]>` the same template when `x == y` at run-time?), not about platform support.
> > > Could be there platforms (on buildbot?) where the VLA code (in cpp file) does not compile? Is it worth to have a test with VLA type alias then?
> > 
> > I think the type alias test is a good idea (as are any other C++-specific tests) because we support VLAs in C++ mode as an extension, my concern was more that this is a C feature and so if I was hunting for tests for VLAs, I'd look  in a .c file first. It's not a huge concern, more like a style nit.
> > 
> > > (The ++ seems to be evaluated for the alignof statement too.)
> > 
> > That seems wrong per C17 6.5.3.4p3. It also doesn't seem to match the behavior I'm seeing: https://godbolt.org/z/rVaGnb
> > 
> There is a //cfg.cpp// test file but not a //cfg.c// so the new tests are added to the cpp file. I do not like to make a new //cfg.c// because it will contain a part of the VLA tests only because the type alias cases. It is better to have all VLA tests in the same file.
> 
> The case in `vla_unevaluated` is probably wrong but an independent problem. This change would be only about adding the VLA related parts.
> There is a cfg.cpp test file but not a cfg.c so the new tests are added to the cpp file. I do not like to make a new cfg.c because it will contain a part of the VLA tests only because the type alias cases. It is better to have all VLA tests in the same file.

I don't see why it matters having all VLA tests in the same file when the file name has nothing to do with VLAs, but if you insist, I can hold my nose.

> The case in vla_unevaluated is probably wrong but an independent problem. This change would be only about adding the VLA related parts.

I'm not super familiar with the analyzer parts of thing, but I'm not certain why this is an independent problem. I thought the purpose to this patch was to ensure VLAs are evaluated when necessary in otherwise unevaluated contexts. The VLA within _Alignof should not be evaluated, so it seems related to this patch. That said, I'm fine if it's handled in a follow-up patch if that's easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77809