kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang/lib/AST/ExprConstant.cpp:10617
+ // copying the data, which is wasteful.
+ for (const unsigned N : {1u, FinalSize}) {
+ unsigned OldElts = Value->getArrayInitializedElts();
----------------
adamcz wrote:
> kadircet wrote:
> > maybe unroll the loop with a helper like `checkConstantInitialization` and
> > just call it for a single element first and perform copy & call again for
> > the whole thing ?
> >
> > that way we can migrate performance issues later on (e.g. put a threshold
> > on `FinalSize`, don't perform the small step check if it's less than 100,
> > since copying might introduce a significant extra latency in such cases).
> >
> > WDYT?
> I thought of that and decided against it. I think the for loop gives us the
> same ability to change the steps - we can just replace {1u, FinalSize} with a
> vector that could have only one element if FinalSize is below some threshold.
>
> Something like:
> std::vector<unsigned> steps;
> if (FinalSize < 100) steps = {FinalSize};
> else steps = {1u, FinalSize};
> for (const unsigned N : steps) {
> [...]
>
> The problem with helper is that it need sooo many arguments:
> N, E, ArrayElt, CAT, Filler, HadZeroInit, Value and "this" (assuming it's
> free function). Seems like it would hurt readability quite a bit.
>
> If you still think it should be a helper, let me know and I won't argue
> further - when it comes to readability the reviewer always knows better :-)
sorry I was actually implying a lambda with the "helper" (should've been more
explicit && at least use the correct style :D), to prevent the plumbing
massacre.
having an earlier logic to decide on the steps if need be also sounds like a
good approach though. so feel free to ignore.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113120/new/
https://reviews.llvm.org/D113120
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
