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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10617
+    // copying the data, which is wasteful.
+    for (const unsigned N : {1u, FinalSize}) {
+      unsigned OldElts = Value->getArrayInitializedElts();
----------------
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 :-)


================
Comment at: clang/lib/AST/ExprConstant.cpp:10641
+          return false;
+        if (Info.EvalStatus.Diag && !Info.EvalStatus.Diag->empty() &&
+            !Info.keepEvaluatingAfterFailure())
----------------
kadircet wrote:
> i am a little worried about having "non fatal/error" diagnostics here. But looking at the documentation of the field, intention seems to be making this empty iff evaluation succeeded so far. so it's probably fine, but might be worth introducing in a separate patch to make the revert easy, in case it breaks things (+ this seems to be an orthogonal change to the rest)
Added a short comment.

I don't think splitting this into separate change makes sense. I'd rather just revert this one. Without this check the whole two-step process is mostly useless, so it's not a state we should be in - either revert the whole thing, or keep it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113120/new/

https://reviews.llvm.org/D113120