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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
