[PATCH] D113120: [clang] Add early exit when checking for const init of arrays.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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