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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
