Page MenuHomePhabricator

[clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values
Needs ReviewPublic

Authored by ayzhao on Jul 11 2022, 6:24 PM.

Details

Reviewers
NoQ
rsmith
ahatanak
erichkeane
aaron.ballman
0x59616e
Group Reviewers
Restricted Project
Summary

This patch implements P0960R3, which allows initialization of aggregates
via parentheses.

As an example:

struct S { int i, j; };
S s1(1, 1);

int arr1[2](1, 2);

This patch also implements P1975R0, which fixes the wording of P0960R3
for single-argument parenthesized lists so that statements like the
following are allowed:

S s2(1);
S s3 = static_cast<S>(1);
S s4 = (S)1;

int (&&arr2)[] = static_cast<int[]>(1);
int (&&arr3)[2] = static_cast<int[2]>(1);

This patch was originally authored by @0x59616e and completed by
@ayzhao.

Fixes #54040, Fixes #54041

Co-authored-by: Sheng <ox59616e@gmail.com>

Full write up : https://discourse.llvm.org/t/c-20-rfc-suggestion-desired-regarding-the-implementation-of-p0960r3/63744

Diff Detail

Unit TestsFailed

TimeTest
11,360 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/ranges/range_adaptors/range_drop_while::base.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.adaptors/range.drop.while/base.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/ranges/range.adaptors/range.drop.while/Output/base.pass.cpp.dir/t.tmp.exe
11,240 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/ranges/range_adaptors/range_drop_while::ctor.view.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.adaptors/range.drop.while/ctor.view.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/ranges/range.adaptors/range.drop.while/Output/ctor.view.pass.cpp.dir/t.tmp.exe
11,100 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.std/ranges/range_adaptors/range_take_while::ctor.view.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e7ac59807d76-1/llvm-project/libcxx-ci/build/generic-cxx2b/test/std/ranges/range.adaptors/range.take.while/Output/ctor.view.pass.cpp.dir/t.tmp.exe
2,720 mslibcxx CI Modules > llvm-libc++-shared-cfg-in.libcxx/algorithms::ranges_robust_against_copying_projections.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/algorithms/ranges_robust_against_copying_projections.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/test/libcxx/algorithms/Output/ranges_robust_against_copying_projections.pass.cpp.dir/t.tmp.exe
3,770 mslibcxx CI Modules > llvm-libc++-shared-cfg-in.std/algorithms::ranges_robust_against_omitting_invoke.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/7e8bb0fbad8e-1/llvm-project/libcxx-ci/build/generic-modules/test/std/algorithms/Output/ranges_robust_against_omitting_invoke.pass.cpp.dir/t.tmp.exe
View Full Test Results (9 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ilya-biryukov added inline comments.
clang/lib/Serialization/ASTReaderStmt.cpp
2175

FYI: I think this is where the crash comes from.
We should allocate the trailing objects first.
E.g. see how PragmaCommentDecl::CreateDeserialized does this.

ayzhao added inline comments.Oct 25 2022, 3:58 PM
clang/lib/Serialization/ASTReaderStmt.cpp
2175

This sounds like it could be the solution - thanks for looking at it!

Currently, I'm working on the refactor that shafik@ suggested, which was to inherit from InitListExpr. Hopefully, that refactor will fix this issue as InitListExpr stores it's subexpressions in a class member instead of using llvm::TrailingObjects.

ilya-biryukov added inline comments.Oct 26 2022, 2:24 AM
clang/lib/Serialization/ASTReaderStmt.cpp
2175

Are we trying to share code between two implementations? If so, I suggest to consider alternatives, e.g. creating a new base class and inheriting both InitListExpr and CXXParentInitListExpr to share the common code.

Inheriting CXXParentInitListExpr from InitListExpr breaks Liskov substitution principle and will likely to lead to bugs that are hard to chase. InitListExpr is widely used and means`{}. CXXParenInitListExpr` is not {} and we do not know in advance which code is going to work for both and which code is only valid for {}. Reviewing all callsites that use InitListExpr does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).

ayzhao updated this revision to Diff 471329.Oct 27 2022, 4:50 PM
ayzhao marked 11 inline comments as done.

Fix PCH test and address some code review comments

Thanks for working on it! It looks really good. Please remember to update the feature test macro (__cpp_aggregate_paren_init).

Done

Also, I think there's no test coverage for the ASTReader/Writer changes? I would like to see some as well.

I added a precompiled header test. Currently, the ASTReader/Writer change is broken with the following error:

...

I'm currently looking into this - I suspect it has to do with CXXParenListInitExpr storing all it's subexpressions in an llvm::TrailingObjects base class, but I have yet to confirm one way or another.

The PCH test is fixed now, so we now have coverage for the ASTReader/Writer changes.

clang/lib/AST/Expr.cpp
3782

See the response from @ilya-biryukov for why I decided against inheriting from InitListExpr.

clang/lib/AST/ExprClassification.cpp
449

IMO; no.

I believe the comment for InitListExpr refers to statements like:

int a;
int &b{a};

This doesn't apply to CXXParenListInitExprClass because the corresponding statement with parentheses wouldn't be parsed as a CXXParenListInitExpr in the first place.

clang/lib/AST/StmtPrinter.cpp
2469

See the response from @ilya-biryukov for why I decided against inheriting from InitListExpr.

clang/lib/Sema/TreeTransform.h
14035

This is an extra change, but clang-format would always make it.

clang/lib/Serialization/ASTReaderStmt.cpp
2175

So I figured out why this was failing. When I created the CXXParenListInitExpr object on the heap in ASTReader::ReadStmtFromStream(...) below, I just called new without allocating any space for the TrailingObjects. This is fixed now, and the PCH test passes.

Inheriting CXXParentInitListExpr from InitListExpr breaks Liskov substitution principle and will likely to lead to bugs that are hard to chase. InitListExpr is widely used and means`{}. CXXParenInitListExpr` is not {} and we do not know in advance which code is going to work for both and which code is only valid for {}. Reviewing all callsites that use InitListExpr does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).

+1, I am convinced that inheriting from InitListExpr is the wrong thing to do here. Additionally, when I tried to inherit from InitListExpr, I found that there were a lot of things in that class that were specific to InitListExpr that I had to wrap with an if statement to make it work with CXXParenListInitExpr, and that the amount of code that I had to add considerably decreases readability.

ayzhao updated this revision to Diff 471338.Oct 27 2022, 5:27 PM

we don't need CHECK-DAG here

ayzhao updated this revision to Diff 471694.Oct 28 2022, 5:59 PM
ayzhao marked 2 inline comments as done.

add C++20 extension diagnostics and some other code review feedback

Friendly ping for reviewers as all comments should be addressed by now.

It starting to look great.
Should we add an extension warning? Note that I'm not suggesting to support that in earlier modes, just an off-by-default warning that says "this is a c++20 feature". but we are a bit inconsistent with those.

Done

can you rename test/SemaCXX/P0960R3.cpp to something like test/SemaCXX/cxx20-paren-list-init.cpp (and same for other tests?) it's more consistent with other existing tests.

Done

clang/lib/Sema/SemaInit.cpp
5272

I added some comments, did a little refactoring, and renamed some variables. PTAL and LMKWYT.

I added a few more comments, mostly nitpicks

clang/lib/AST/ExprConstant.cpp
9972–9973
9976

Maybe that message could be a bit more clear.

"Union should have exactly 1 initializer in in C++ paren list" or something like that.

clang/lib/CodeGen/CGExprAgg.cpp
1603

There are some overlaps with VisitInitListExpr, I wonder if we should try to factor out some of the code, especially the base class initialization

1608

Maybe it would be cleaner to use a structured binding here.

1624

Should we assert that the base is not virtual?

clang/lib/Sema/SemaInit.cpp
5291
clang/lib/Sema/TreeTransform.h
14043

ArgumentChanged is not used, and is defaulted by the function, so you should be able to remove it

ayzhao updated this revision to Diff 472190.Oct 31 2022, 5:33 PM
ayzhao marked 6 inline comments as done.

address some comments and implement disallowing flexible array members

Status update:

While investigating @cor3ntin's comment about refactoring VisitInitListExpr and VisitCXXParenListInitExpr as they share common code, I discovered that flexible array members would cause this patch to explode. Specifically, the following code:

struct S {
    int b;
    int a[];
};

S s2(1, {1, 2});

was causing this assertion to fail.

I took a look at GCC, and it turns out that GCC doesn't allow flexible array members at all in paren list initialization of aggregates, even in contexts where the corresponding designated initializer expression succeeds: https://godbolt.org/z/E73433erb

For now, I updated this implementation to follow GCC's behavior and disallow flexible array members, as IIRC in C++ they are a GCC extension and we should therefore follow GCC behavior. However, I am open to other thoughts.

ayzhao updated this revision to Diff 472405.Nov 1 2022, 2:45 PM

clang-format + rebase

ayzhao updated this revision to Diff 472441.Nov 1 2022, 4:11 PM
ayzhao marked an inline comment as done.

Merge CXXParenListInitExpr with VisitInitListExpr, also friendly ping as all comments are addressed

I think this is basically good to go, but I'll let other people give it a look too

clang/lib/AST/ExprConstant.cpp
9985–9988
clang/lib/CodeGen/CGExprAgg.cpp
91

Could you give E a better name?

484–486

Ditto

ayzhao updated this revision to Diff 472702.Nov 2 2022, 11:08 AM
ayzhao marked 3 inline comments as done.

addressed code review comments

ayzhao updated this revision to Diff 473746.Mon, Nov 7, 10:51 AM

rebase + update commit message

ayzhao retitled this revision from [clang][C++20] P0960R3: Allow initializing aggregates from a parenthesized list of values to [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values.Mon, Nov 7, 10:53 AM
ayzhao edited the summary of this revision. (Show Details)

Friendly ping, does anyone else have any more comments?

ayzhao updated this revision to Diff 473772.Mon, Nov 7, 1:35 PM

whoops, uploaded the wrong commit. should be fixed now

ayzhao updated this revision to Diff 473779.Mon, Nov 7, 1:44 PM

remove extra parens

ayzhao edited the summary of this revision. (Show Details)Mon, Nov 7, 1:45 PM
ayzhao updated this revision to Diff 473789.Mon, Nov 7, 2:08 PM

s/pro20/post20/g

ayzhao edited the summary of this revision. (Show Details)Mon, Nov 7, 4:06 PM
danakj added a subscriber: danakj.Mon, Nov 7, 5:28 PM

There is a C++ standard meeting this week so some folks won't be able to review it. Feel free to re-ping next week if nobody else replies.
Because it's a fairly big change and I've been known to miss glaring things, I don't feel comfortable to approve it. But it looks great to me.
Either way we need to make sure this ships in Clang 16.

I am in the committee meeting this week but if you ping next week I should have time.

Sorry for not reviewing this sooner. This looks very good overall, but I still have some NITs and a few major comments. For the major points see:

  • the comment about the filler and the syntactic/semantic form for the newly added expression,
  • the comment about relying on getFailedCandidateSet().size().
clang/lib/AST/ExprCXX.cpp
1777

NIT: add newline

clang/lib/AST/ExprConstant.cpp
9985

NIT: maybe add braces as the other branches have them? This seems to be in the style guide:

readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members

10968

Could we add an assertion that the array size and the InitExprs.size() are the same?
E.g. it can differ in the case of InitListExpr (probably due to the filler optimization?), it would be nice to add a sanity check here.

clang/lib/AST/JSONNodeDumper.cpp
853

NIT: maybe use the same formatting as other switch cases for constistency?

clang/lib/CodeGen/CGExprAgg.cpp
484–486

NIT: maybe add a comment that ExprToVisit is either InitListExpr or CXXParenInitListExpr?

585
  • Should we have a filler for CXXParenInitListExpr too? It seems like an important optimization and could have large effect on compile times if we don't
  • Same question for semantic and syntactic-form (similar to InitListExpr): should we have it here? I am not sure if it's semantically required (probably not?), but that's definitely something that clang-tidy and other source tools will rely on.

We should probably share the code there, I suggest moving it to a shared base class and using it where appropriate instead of the derived classes.

clang/lib/Sema/SemaInit.cpp
6223

It seems shaky to rely on the size of getFailedCandidateSet.
What are we trying to achieve here? Is there a more direct way to specify it?

6242

NIT: maybe move the full comment into the body of the function?
It describes in detail what the body of the function does and it would be easier to understand whether the code fits the intention in the standard.
Having the first part of the comment here would still be useful, probably.

clang/lib/Serialization/ASTReaderStmt.cpp
2174

Could we assign the CXXParenInitListExpr.NumExprs on construction and add the assertion sanity-checking the numbers here:

unsigned NumExprs = Record.readInt();
(void)NumExprs; // to avoid unused warnings in NDEBUG builds.
assert(NumExprs == E->NumExprs);

(Looked up how DesignatedInitExpr does a similar thing, probably good for consistency in addition to added safety)

ayzhao updated this revision to Diff 476625.Fri, Nov 18, 4:01 PM
ayzhao marked 9 inline comments as done.

address code review comments

clang/lib/AST/JSONNodeDumper.cpp
853

Unfortunately clang-format insists that these be on separate lines.

clang/lib/CodeGen/CGExprAgg.cpp
585

Should we have a filler for CXXParenInitListExpr too? It seems like an important optimization and could have large effect on compile times if we don't

This feels like premature optimization - presumably, wouldn't this only be an issue with extraordinarily large (say, O(1000)) arrays?

Same question for semantic and syntactic-form (similar to InitListExpr): should we have it here? I am not sure if it's semantically required (probably not?), but that's definitely something that clang-tidy and other source tools will rely on

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic form would be the enclosing ParenListExpr.

clang/lib/Sema/SemaInit.cpp
6223

Done.

As mentioned in the above comment, we try to parse this as a CXXParenListInit if the only failed overload constructor candidates are the default constructor, the default copy constructor, and the default move constructor).

6242

Done. Most of this comment is spread out throughout the body of the function anyways.

ychen added a subscriber: ychen.Sun, Nov 27, 10:12 PM

Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but it looks like we actually loose the syntactic form completely in this case.

clang/lib/AST/JSONNodeDumper.cpp
853

Ah, that's unfortunate. I normally just revert the effect of clang-format for those lines, but up to you.
Also fine to keep as is or even format the other lines according to the style guide (it's just 3 more lines, so should not be a big deal).

clang/lib/CodeGen/CGExprAgg.cpp
585

This feels like premature optimization - presumably, wouldn't this only be an issue with extraordinarily large (say, O(1000)) arrays?

Yes, this should only happen with large arrays. Normally I would agree, but it's surprising that changing {} to () in the compiler would lead to performance degradation.
In that sense, this premature optimization is already implemented, we are rather degrading performance for a different syntax to do the same thing.

Although we could also land without it, but in that case could you open a GH issue and add a FIXME to track the implementation of this particular optimization?
This should increase the chances of users finding the root cause of the issue if they happen to hit it.

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic form would be the enclosing ParenListExpr.

Do we even have the enclosing ParenListExpr? If I dump the AST with clang -fsyntax-only -Xclang=-ast-dump ... for the following code:

struct pair { int a; int b = 2; };
int main() {
  pair(1); pair p(1);
  pair b{1}; pair{1};
}

I get

`-FunctionDecl 0x557d79717e98 <line:2:1, line:5:1> line:2:5 main 'int ()'
  `-CompoundStmt 0x557d797369d0 <col:12, line:5:1>
    |-CXXFunctionalCastExpr 0x557d79718528 <line:3:3, col:9> 'pair':'pair' functional cast to pair <NoOp>
    | `-CXXParenListInitExpr 0x557d79718500 <col:3> 'pair':'pair'
    |   |-IntegerLiteral 0x557d79718010 <col:8> 'int' 1
    |   `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
    |-DeclStmt 0x557d79718650 <line:3:12, col:21>
    | `-VarDecl 0x557d79718568 <col:12, col:17> col:17 p 'pair':'pair' parenlistinit
    |   `-CXXParenListInitExpr 0x557d79718610 <col:17> 'pair':'pair'
    |     |-IntegerLiteral 0x557d797185d0 <col:19> 'int' 1
    |     `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
    |-DeclStmt 0x557d797187d8 <line:4:3, col:12>
    | `-VarDecl 0x557d79718680 <col:3, col:11> col:8 b 'pair':'pair' listinit
    |   `-InitListExpr 0x557d79718750 <col:9, col:11> 'pair':'pair'
    |     |-IntegerLiteral 0x557d797186e8 <col:10> 'int' 1
    |     `-CXXDefaultInitExpr 0x557d797187a0 <col:11> 'int'
    `-CXXFunctionalCastExpr 0x557d797369a8 <col:14, col:20> 'pair':'pair' functional cast to pair <NoOp>
      `-InitListExpr 0x557d79718868 <col:18, col:20> 'pair':'pair'
        |-IntegerLiteral 0x557d79718800 <col:19> 'int' 1
        `-CXXDefaultInitExpr 0x557d797188b8 <col:20> 'int'

It feels like the ParentListExpr is replaced during semantic analysis and there is no way to get it back. I also tried running clang-query and trying to match parenListExpr() and go 0 results. Is it just missing in the AST dump and I run clang-query incorrectly or do we actually not have the syntactic form of this expression after all?

ilya-biryukov added inline comments.Mon, Nov 28, 8:05 AM
clang/lib/CodeGen/CGExprAgg.cpp
585

Another important thing to address from the dump: notice how braced initialization creates CXXDefaultInitExpr and CXXParenListInitExpr copies the default argument expression directly. It's really important to use the former form, here's the example that currently breaks:

#include <iostream>

struct func_init { int some_int; const char* fn = __builtin_FUNCTION(); };

int main() {
  func_init a(10);
  std::cout << "From paren init:" << a.fn << std::endl;

  func_init b{10};
  std::cout << "From braced init: " << b.fn << std::endl;
}

The program above is expected to report main for both cases, but instead we get:

From paren init:
From braced init: main
ayzhao updated this revision to Diff 479758.Fri, Dec 2, 3:15 PM
ayzhao marked 2 inline comments as done.

implement CXXDefaultInitExpr and ImplicitValueInitExpr, array fillers, and semantic vs syntatic forms.

clang/lib/CodeGen/CGExprAgg.cpp
585

The following have now been implemented:

  • CXXDefaultInitExpr and ImplicitValueInitExpr, which includes adding a test for __builtin_FUNCTION()
  • Array fillers
  • Semantic forms vs syntactic forms