Skip to content

modernize-use-designated-initializers reported for pre-C++20 code #83732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
firewave opened this issue Mar 3, 2024 · 14 comments · Fixed by #94651
Closed

modernize-use-designated-initializers reported for pre-C++20 code #83732

firewave opened this issue Mar 3, 2024 · 14 comments · Fixed by #94651
Assignees

Comments

@firewave
Copy link

firewave commented Mar 3, 2024

struct S
{
    int i1;
    int i2;
};

void f()
{
    S s{0};
}

Running with -std=c++11 produces the following warning:

<source>:9:8: warning: use designated initializer list to initialize 'S' [modernize-use-designated-initializers]
    9 |     S s{0};
      |        ^~~
      |         .i1=
<source>:1:1: note: aggregate type is defined here
    1 | struct S
      | ^

https://godbolt.org/z/EzerEWsKh

Designated initializers are not available until C++20: https://en.cppreference.com/w/cpp/language/aggregate_initialization.

CC @SimplyDanny @PiotrZSL

@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2024

@llvm/issue-subscribers-clang-tidy

Author: Oliver Stöneberg (firewave)

```cpp struct S { int i1; int i2; };

void f()
{
S s{0};
}


Running with `-std=c++11` produces the following warning:

<source>:9:8: warning: use designated initializer list to initialize 'S' [modernize-use-designated-initializers]
9 | S s{0};
| ^~~
| .i1=
<source>:1:1: note: aggregate type is defined here
1 | struct S
| ^


https://godbolt.org/z/EzerEWsKh

Designated initializers are not available until C++20: https://en.cppreference.com/w/cpp/language/aggregate_initialization.

CC @<!-- -->SimplyDanny @<!-- -->PiotrZSL 
</details>

@EugeneZelenko
Copy link
Contributor

@SimplyDanny

@SimplyDanny
Copy link
Member

In the review, @PiotrZSL noted that designated initializers often work even with older compiler versions. That's why we decided against the restriction to C++20.

@firewave
Copy link
Author

firewave commented Mar 3, 2024

Interesting - I wasn't aware of that.

A short test show they actually do work in C++11 mode with Clang 3.5 and GCC 5.1 which are the oldest compilers we need to support.

But they do not work with Visual Studio until cl v19.21 (which appears to be Visual Studio 2019 16.1) and only if you enable /std:c++latest. Visual Studio 2017 is still supported until 2027 and the oldest we still have to support is VS2015.

Unfortunately there is no AppleClang in Godbolt so I cannot test if they are available in that.

@carlosgalvezp
Copy link
Contributor

carlosgalvezp commented Mar 3, 2024

often work even with older compiler versions

That's not true if you enable -pedantic (a.k.a Standards conformat, which most projects should do). Please ensure this warning is only enabled on C++20, this feature is not available until then.

@PiotrZSL
Copy link
Member

PiotrZSL commented Mar 3, 2024

That's not true if you enable -pedantic (a.k.a Standards conformat, which most projects should do). Please ensure this warning is only enabled on C++20, this feature is not available until then.

We use it with C++17 now, and before that we used it even before C++11 as GCC extension.
With latest Clang you can use "-std=c++17 -Wall -pedantic -Wno-c++20-designator", and everything work fine.

https://godbolt.org/z/77d5jM7T3

You can use it also in C: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

@carlosgalvezp
Copy link
Contributor

carlosgalvezp commented Mar 3, 2024

Of course, that's disabling the warning :) A clang-tidy check should not require people to disable compiler warnings.

Designated initializers is a C++20 feature according to the C++ Standard. The fact that compilers implement it earlier is a compiler extension. Compiler extensions are forbidden in certain coding standards, e.g. MISRA, AUTOSAR, etc. That's why -pedantic exists, to ensure no compiler extensions are enabled and the code is Standards-conformant.

Yes, this feature is part of the C standard since long time ago, only made it into C++ in C++20 for some reason.

If this is wanted, it should be handled as an opt-in option (off by default).

@carlosgalvezp
Copy link
Contributor

WDYT @AaronBallman @njames93 ?

@PiotrZSL
Copy link
Member

PiotrZSL commented Mar 5, 2024

I see 2 options:
a) This check is not enabled by default, so user may not enable it if not needed.
b) If user use modernize-* to enable all checks, then in theory some option like "EnableBeforeCpp20" (or something else) could be added, but still modernize-* got already other checks like modernize-use-trailing-return-type that probably user may want to disable anyway.

@AaronBallman
Copy link
Collaborator

I don't think it's a problem to recommend using designated initializers in pre-C++20 mode as that's a documented (https://clang.llvm.org/docs/LanguageExtensions.html#language-extensions-back-ported-to-previous-standards), conforming language extension we support. By default, I think we should silence modernize-use-designated-initializers in C++20 mode as that's the behavior of other modernization checks.

That said, I think it's reasonable to give users control over whether they get the recommendation in pre-C++20 mode or not. We could use a configuration option for this, or we could explore making clang-tidy aware of -pedantic mode being enabled in Clang (and disable the recommendation in -pedantic mode). For example modernize-unary-static-assert, modernize-use-nodiscard, and modernize-use-auto are all disabled in older modes but the extensions are supported by Clang in older modes. It would be nice to have a consistent option for users so they don't have to configure each check if they're comfortable relying on extensions.

@PiotrZSL
Copy link
Member

PiotrZSL commented Mar 7, 2024

Maybe then some generic option that would be added by default to every check to skip language checks for that check. In such case user could configure: modernize-use-auto.SkipLanguageCheck: On or something like that or something like with WarningsAsErrors option.

@carlosgalvezp
Copy link
Contributor

Yes, a global option makes sense. I don't foresee the need to configure this on a per-check basis. I think making it smarter by checking the -pedantic flag introduces a dependency towards the compiler that might be undesirable and complicate things down the road.

I suppose it's now time to bikeshed about the name :) What about CppStandardCompliance: strict/relaxed or simply StrictCppStandardCompliance: on?

@PiotrZSL
Copy link
Member

PiotrZSL commented Mar 8, 2024

StrictCppStandardCompliance sounds better, default true/on, but only as a config option, not as an program parameter.
But I'm not sure if it can be implemented, image checks that are only for C or only for Objective-C, or only for C++. Because all checks are done in isLanguageVersionSupported we do not know what are check requirements. This is why I wanted this option to be an list of checks like: SkipLanguageComplianceChecks: "modernize-use-auto,modernize-unary-static-assert" or be a magic option auto-generated for every check.

@johnmcfarlane
Copy link

Designated initializers are a great feature. For projects and with toolchains where deviating from the standard is possible, I'd certainly consider using this feature ahead of upgrading to C++20. Unfortunately, there are plenty of scenarios where this isn't possible. It's common for software in some safety-critical domains to be locked into C++14 and GCC-based toolchains. For those projects, a check that actively breaks conforming code isn't welcome. I don't want to have to deviate from this rule; it's valid, just not applicable to me at this time.

So whatever solution is chosen, please can we make it an opt-in?

Other checks observe standards conformance. Casually enabling C++20 in my own project, the following well-behaved checks start to trigger:

  • bugprone-reserved-identifier
  • cert-dcl37-c
  • cert-dcl51-cpp
  • modernize-concat-nested-namespaces
  • modernize-type-traits
  • modernize-use-constraints
  • modernize-use-designated-initializers

I am grateful to the authors of those checks for making sure they apply to my combination of compiler flags.

@PiotrZSL PiotrZSL self-assigned this Jun 6, 2024
PiotrZSL added a commit to PiotrZSL/llvm-project that referenced this issue Jun 6, 2024
…tions

Add StrictCStandardCompliance and StrictCppStandardCompliance
options that default to true.

Closes llvm#83732
PiotrZSL added a commit that referenced this issue Jun 9, 2024
…tions (#94651)

Add StrictCStandardCompliance and StrictCppStandardCompliance options
that default to true.

Closes #83732
nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this issue Jun 9, 2024
…tions (llvm#94651)

Add StrictCStandardCompliance and StrictCppStandardCompliance options
that default to true.

Closes llvm#83732

Signed-off-by: Hafidz Muzakky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants