rturrado wrote:
> > > I don't believe this is a good change sorry - it's a relatively large
> > > abstraction layer to "simplify" maybe two actual expressions.
> > > If we really wanted to do something here I would probably just replace
> > > the optional<> with
> > > ```c++
> > > bool IsArraySizeDependent = false;
> > > bool ... = false;
> > > if (ArraySize) {
> > > set all the "there is an array size" variables as appropriate
> > > }
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > It seems the main concern is the compound conditions, which is consistent
> > > with what other code does, but I would prefer to reduce that in general.
> > > But the issue here is not made better with an abstraction (the better
> > > option would be actual language support for optional in c++)
> > > I've marked as "request changes" but I think this is probably the wrong
> > > tack, and I'm not sure the problem with new/delete Sema is this bit.
> > > There's so much more that is so much worse.
> > > While not an option for clang any time soon, I have wondered if
> > > reflection could be used in a way to make this kind of thing more
> > > expensive.
> >
> >
> > Many thanks for the thorough review. And from my side, same comments I did
> > to @tbaederr. You're very right that this current code is overkill. I will
> > explore the variant solution.
>
> Have a quick look at just turning the various checks into a set of bools
> first and see what that does to the overall appearance. I'll try to refresh
> exactly what it was I didn't like about array size specifically as there's a
> lot of clunkiness in this code in general (I really wish I put down more
> information at the time)
I have updated the PR with a minimal change mainly involving some boolean
variables definitions. I understand if that wouldn't be worth a PR!
Notice `std::optional<Expr *> ArraySize` is assigned to at different points of
`BuildCXXNew` so even these boolean variables need to be recalculated.
I also played a bit with a `std::variant<std::monostate, std::nullptr_t, Expr
*> ArraySize` version, but the code was quite more bloated than current one.
https://github.com/llvm/llvm-project/pull/186617
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
