Page MenuHomePhabricator

[clang] Try to improve diagnostics about uninitialized constexpr variables
ClosedPublic

Authored by tbaeder on Aug 11 2022, 2:35 AM.

Details

Summary

Consider:

constexpr int a;

the error message for this is currently:

error: default initialization of an object of const type 'const int'
constexpr int a;
              ^
                = 0

which makes very little sense to me. With this patch, the output is instead:

error: constexpr variable 'a' must be initialized by a constant expression
constexpr int a;
              ^
                = 0

which is much better. Tells the user exactly what is missing.

For

constexpr int a[];

before this patch, the output is:

error: definition of variable with array type needs an explicit size or an initializer
constexpr int a[];
              ^

The error message is not even true in this case, since _only_ passing a size won't work. It must be initialized by a constant expression, so now the error message is:

error: constexpr variable 'a' must be initialized by a constant expression
constexpr int a[];
              ^

and a fixit hint if a size was provided:

error: constexpr variable 'a' must be initialized by a constant expression
constexpr int a[2];
              ^
                   = {}

(not sure about that part)

Diff Detail

Event Timeline

tbaeder created this revision.Aug 11 2022, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 2:35 AM
tbaeder requested review of this revision.Aug 11 2022, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 2:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for working on this, I like the direction it's heading! One question I'm kicking around my head is, should we do something similar for const variables?

const int i; // error: default initialization of an object of const type 'const int'

it sure seems like this could be similarly improved to say something along the lines of error: variable 'i' of const type must be initialized

Also, don't forget to add a release note.

clang/lib/Sema/SemaFixItUtils.cpp
208–210 ↗(On Diff #451780)

I don't think this is a good change, consider:

int array[] = {};

zero-sized arrays are an extension in both C and C++, and the empty initializer is a GNU extension in C (at least until C2x).

clang/lib/Sema/SemaInit.cpp
8063–8078

Why the check for a record type?

tbaeder marked an inline comment as done.Aug 17 2022, 9:55 PM
tbaeder added inline comments.
clang/lib/Sema/SemaInit.cpp
8063–8078

For record types, the current error message would be:

default initialization of an object of const type 'const S3' without a user-provided default constructor

which gives more information than just "must be initialized by a constant expression".

tbaeder added inline comments.Aug 17 2022, 9:58 PM
clang/lib/Sema/SemaFixItUtils.cpp
208–210 ↗(On Diff #451780)

The code in SemaInit.cpp is weird in that it only holds if the fixit isn't empty... so if that change does away, a constexpr int arr[2]; also doesn't use the new error message :(

aaron.ballman added inline comments.Aug 18 2022, 7:23 AM
clang/lib/Sema/SemaFixItUtils.cpp
208–210 ↗(On Diff #451780)

Ugh, well that's rather unfortunate. But I'm not certain the improved diagnostic is worth the incorrect fix-it unless the array has specified bounds. Perhaps another way to address this is to work on the diagnostic behavior to be more like:

error: definition of constexpr variable with array type needs an explicit initializer
constexpr int a[];
              ^

Does that seem more plausible?

clang/lib/Sema/SemaInit.cpp
8063–8078

Yeah, that is a better diagnostic, thanks! Too bad the diagnostic logic is separated like this though.

tbaeder updated this revision to Diff 453664.Aug 18 2022, 8:12 AM
tbaeder marked 2 inline comments as done.

The new version also gets the new error message for constexpr int foo[2];.

aaron.ballman accepted this revision.Aug 18 2022, 10:04 AM

LGTM, though you should add a release note.

clang/lib/Sema/SemaInit.cpp
8057
8063–8078
9477

Oh, neato, I see dyn_cast_or_null was deprecated...

This revision is now accepted and ready to land.Aug 18 2022, 10:04 AM
tbaeder marked 3 inline comments as done.Aug 18 2022, 11:06 PM
This revision was landed with ongoing or failed builds.Aug 18 2022, 11:06 PM
This revision was automatically updated to reflect the committed changes.