This is an archive of the discontinued LLVM Phabricator instance.

Implementation of VlA of GNU C++ extension
ClosedPublic

Authored by vbyakovl on Apr 6 2016, 5:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

vbyakovl retitled this revision from to Implementation of VlA of GNU C++ extension.
vbyakovl updated this object.
vbyakovl added a reviewer: aaron.s.wishnick.

The patch itself looks good to me but the main question do we want to support VLA in C++ in GNU extension mode.

Adding a few more reviewers.

Is there a major use case driving this patch, or is this just for GCC compatibility? Personally, I would prefer to avoid adding VLA support to C++ unless there's a good motivating use case, but would like to hear what Richard or Doug think on the subject.

En passant comment: I really wish we wouldn't.

The C++ standard had some very careful considerations on VLAs and decided *not* to support. It wasn't for lack of interest, it was a well informed decision.

llvm/tools/clang/test/SemaCXX/c99-variable-length-array.cpp
1 ↗(On Diff #52779)

are we not supporting this in c99 anymore?

llvm/tools/clang/test/SemaCXX/vla.cpp
1 ↗(On Diff #52779)

doesn't this remove the test for support for it in c99?

DmitryPolukhin added inline comments.Apr 11 2016, 7:57 AM
llvm/tools/clang/test/SemaCXX/c99-variable-length-array.cpp
1 ↗(On Diff #52779)

No, it is C++ test due to file extension .cpp. So C99 mode shouldn't be affected as far as I understand (i.e. the test was never run in C99). Mode by default is gnu++98 where this patch enable VLA so some warnings/errors from this test are gone (-Wvla-extension).

En passant comment: I really wish we wouldn't.

The C++ standard had some very careful considerations on VLAs and decided *not* to support. It wasn't for lack of interest, it was a well informed decision.

The decision might have been well informed, but it was really a lack of consensus. There was (and still is) a large fraction of the committee in favor of VLAs (or ARBs as they called them). There is also a large fraction of the committee opposed to them, or willing to accept them, but only if we could also add a library type with a better interface. There is a concern, which is not completely unreasonable, that if we make a point of adding this feature to the C++ standard, without adding a library container type, it will make people more likely to use "raw" arrays and pointers instead of using higher-level containers and iterators. It turns out, however, that it is not clear how such a library container could be implemented in practice.

However, as an implementation extension, this concern is not relevant. I'm in favor of this; I have uses who use this feature in GCC. It is certainly true that most HPC users are using these on PODs, but the fact that you cannot apply them to other types creates a problem with generic programming that people have run into.

However, as an implementation extension, this concern is not relevant. I'm in favor of this; I have uses who use this feature in GCC. It is certainly true that most HPC users are using these on PODs, but the fact that you cannot apply them to other types creates a problem with generic programming that people have run into.

Sounds odd to argue in favour of generic programming and not in favour of using generic standard containers.

I can easily accept the legacy argument, but creating new code with contentious features seems totally backwards to me.

However, as an implementation extension, this concern is not relevant. I'm in favor of this; I have uses who use this feature in GCC. It is certainly true that most HPC users are using these on PODs, but the fact that you cannot apply them to other types creates a problem with generic programming that people have run into.

Sounds odd to argue in favour of generic programming and not in favour of using generic standard containers.

I can easily accept the legacy argument, but creating new code with contentious features seems totally backwards to me.

It comes up in cases where you're writing templates that are normally used with PODs and the relevant types, but don't want them to completely fail when used with other types (class for extended precision floats, or those doing forward automated differentiation, etc.).

Using VLAs in C++, as a general statement, is one of the most common language extensions I see used. It is used in new code regularly, and is often critical for performance (i.e. to avoid calls to the memory allocator). Using them on on-PODs is certainly less common, but still comes up.

rsmith edited edge metadata.Apr 11 2016, 12:51 PM

@vbyakovl The description of this patch is very misleading. What I believe this does is to allow VLAs with non-trivially-destructed element types. Can you fix the description (and make sure the fixed version ends up in the commit message if this is approved)?
@rjmccall Do you recall why we disallow this?

rjmccall edited edge metadata.Apr 11 2016, 1:19 PM

It requires careful IRGen support which at one point did not exist. When I implemented VLAs of strong/weak references for ARC, I deliberately tried to make sure that the infrastructure would also work for the C++ cases, but I didn't have time to go actually test any of it. It looks like the support was nearly sufficient.

rsmith added inline comments.Apr 11 2016, 2:19 PM
llvm/tools/clang/lib/Sema/SemaType.cpp
2158–2159 ↗(On Diff #52779)

This isn't right; this is *not* a GNU C++ extension. GCC rejects it with -pedantic-errors in GNU and non-GNU mode, and accepts it by default in GNU and non-GNU mode.

What you need to do is to delete the check for a POD element type below, since that is the new feature you're adding support for.

Then please revert all of your test changes other than the new file.

doug.gregor edited edge metadata.Apr 11 2016, 11:04 PM

I think it's completely reasonable to implement support for VLAs as a GNU C++ extension. We did go through a phase where we tried to avoid implementing VLAs in C++ because we considered them to be a poor feature in C++. However, their use was wide-spread enough that we changed course and enabled the implementation for POD types in C++. That got us most of the compatibility without a significant amount of effort, whereas we didn't have the infrastructure to handle non-PODs at that time. It wasn't a statement of intent---it just wasn't important enough to implement at the time. Looks like rjmccall's work on VLAs containing ARC-qualified pointers got us most of the way there, so it makes sense to generalize it to non-POD types.

vbyakovl edited edge metadata.

Sema/SemaType.cpp: Vla works in C++ by default. An error is printed if a class has not default constructor.
test/SemaCXX/c99-variable-length-array.cpp: Changes is removed
test/SemaCXX/vla.cpp: changes is removed.
test/CodeGenCXX/vla-consruct.cpp is added.

rsmith added inline comments.Apr 13 2016, 1:49 PM
llvm/tools/clang/lib/Sema/SemaType.cpp
2163–2165 ↗(On Diff #53548)

Why reject types without default constructors? That's something SemaInit should be checking for, not the responsibility of this code. If we really need to reject them here for some reason, a "non-POD" error is not the right way to diagnose it.

Sema/SemaType.cpp: Deleted POD type and default constructor checkings.
test/SemaCXX/c99-variable-length-array-cxx11.cpp: Edited dianocnics.

Richard, now my changes are good for you?

rsmith accepted this revision.Apr 28 2016, 4:10 PM
rsmith edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Apr 28 2016, 4:10 PM
This revision was automatically updated to reflect the committed changes.