[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

Wed May 25 09:20:52 PDT 2022

aaron.ballman added a comment.

In D126324#3537282 <https://reviews.llvm.org/D126324#3537282>, @jyknight wrote:

> In D126324#3536785 <https://reviews.llvm.org/D126324#3536785>, @aaron.ballman wrote:
>
>> The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks.
>
> I don't feel this is really necessary.

Why?

My thinking is: it's invalid to initialize a file scope variable in C from something other than a constant expression. These are not constant expressions. Why should the initialization work silently and maybe do something wrong?

>> 2. Using a const weak symbol in a constant expression context should probably be an error, right? e.g.,
>
> That's already the case (except, using a VLA is not an error in either language mode). Clang previously _only_ treated weak symbols as compile-time-known in the backend; the frontend has treated it as an non-constant-evaluable variable for ages, and that behavior is unchanged by this patch.

Ah, okay, then it sounds like we already have the behavior I was hoping for, great.

>> Also, this definitely could use a release note so users know about the behavioral change.
>>
>> Do you have any ideas on how much code this change is expected to break? (Is it sufficient enough that we want to have an explicit deprecation period of the existing behavior before we switch to the new behavior?)
>
> Because it's only changing the optimizer behavior not the frontend constant evaluator, I think it's pretty safe (which is also why I'm OK with it).

+1

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126324