Origami404 added a subscriber: mizvekov. Origami404 added a comment. Hello, I am back now and will be available anytime next week, so if anyone has any idea on this topic, please at me!
I haven't found a way that "feels right" to achieve consensus yet. But I do have something to share currently. --- For the semantics of the warning flag, I agree with @uecker that `-Wvla-portability` should be changed to `-Wc++-compat`. In my reading of C2x standard, using VM type is acceptable and the only reason we need to issue a warning is that the C++ standard doesn't allow non-constant array length at all. But I have no opinion on the semantics of `-Wvla` and `-Wvla-stack-alloc`. And should `-Wvla-stack-alloc` fires on static VLAs? (like `clang/test/drs/dr3xx.c:164`) --- For implement details, I add a diag at ``, which will be called after a variable declaration. Here we can get the full type of the variable to tell whether the variable has a VLA type or just a VM type. In the GitHub issue <https://github.com/llvm/llvm-project/issues/57098#issuecomment-1265130710>, @mizvekov said: > A general check about any VLA usage at all should probably happen early when > we finished parsing and are building VariableArray types, probably in > `Sema::BuildArrayType`. > > Otherwise sprinkling around checks for VM types seems messy, so I don't think > considering VM for this problem is an efficient approach. But AFAIK, a VLA type cannot be told just at `Sema::BuildArrayType` level because the constructing VLA type may just be a part of another VM type but not a VLA type for a VLA. At `Sema::BuildArrayType`, we seem only can detect VM type instead of VLA type. --- In Ballman's summary, the `-Wvla-stack-alloc` should suppress the `-Wvla-portability`: > -Wvla-portability warns on any use of a VM type, except if > -Wvla-stack-allocation is also enabled it won't warn on the use of VLA that > involves a stack allocation I failed to achieve the "except" here because, in the current implementation, VM type-related warnings are checked directly at `Sema::BuildArrayType`, by `checkArraySize` which used `Sema::VerifyIntegerConstantExpression`. But the `Sema::VerifyIntegerConstantExpression` requires at least one diag at each non-ICE occurrence. I haven't found a way to avoid the diag yet. --- By the way, I seem to find a bug in the current implementation of VM type warning. For VLA at sizeof context, the warning will be issued twice. Github issue here <https://github.com/llvm/llvm-project/issues/59570>. It cause the test `clang/test/Sema/warn-vla.c:48` giving two warnings. --- The reason why I set both `-Wvla-stack-alloc` and `-Wvla-portability` to `DefaultIgnore` is that: - if both of them are not `DefaultIgnore`, 200 tests will fail - if only `-Wvla-stack-alloc` is not `DefaultIgnore`, 169 tests will fail - if both of them are `DefaultIgnore`, only 4 tests will fail Most of the failed tests seem cannot be avoided because they just use VLAs. If `DefaultIgnore` is strongly unrecommended, I can go through all the tests and add an expected warning diag for them. It's a bit time-consuming, but I am free all day now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137343/new/ https://reviews.llvm.org/D137343 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
