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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks this looks amazing! i am also not an expert in these parts of the code but AFAICT the proposed behavior is in line with the contract. i am a little worried about the cost of extra copy (especially when there are only a handful of elements), but it should be probably fine.



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


================
Comment at: clang/lib/AST/ExprConstant.cpp:10641
+          return false;
+        if (Info.EvalStatus.Diag && !Info.EvalStatus.Diag->empty() &&
+            !Info.keepEvaluatingAfterFailure())
----------------
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)


================
Comment at: clang/test/SemaCXX/PR51712-large-array-constexpr-check-oom.cpp:2
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
----------------
maybe test with `ulimit` and ~1 GB of memory? (you might need `Requires: shell` and `unsupported: windows`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113120