ayzhao marked 4 inline comments as done.
ayzhao added a comment.
In D129531#3971392 <https://reviews.llvm.org/D129531#3971392>, @ilya-biryukov
wrote:
> Thanks!
>
> I have two major comments and also inline NITs. Not sure if we should block
> on those, just wanted to hear your opinions:
>
> - `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code
> duplication is substantial and I think sharing the common implementation is
> warranted. E.g. if some code would want to change something with
> `ArrayFiller` or make a use of it, the work will probably need to be
> duplicated. To reiterate what I mentioned earlier, I believe deriving
> `CXXParenInitListExpr` from `InitListExpr` is not a very good option, but we
> might explore other possibilities: a base class or factoring out common code
> in a separate struct. I believe this could be done with a follow-up change,
> though, as the amount of changes required does not seem too big, I would be
> happy with first landing this patch and then cleaning up with a follow-up
> change.
> - Did we explore the possibility of modelling this differently and somehow
> avoiding making `CXXParenInitListExpr` an expression. This seems to borrow
> from `InitListExpr`, but maybe for the wrong reasons. Braced initializers
> can actually occur in expressions positions, e.g. `int a = {123}`. However,
> `CXXParenInistListExpr` cannot occur in expression positions outside
> initializations, e.g. having `CXXFunctionalCastExpr` for
> `aggregate_struct(123)` feels somewhat wrong to me and seems to lead to
> confusing error messages too. Maybe we should model it as a new expression
> that contains both the type and the argument list? I.e. more similar to
> `CXXConstructExpr`?
As discussed offline, let's keep the current implementation as is for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129531/new/
https://reviews.llvm.org/D129531
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
