[clang-tidy] Add new check `modernize-use-designated-initializers` #80541

SimplyDanny · 2024-02-03T13:27:09Z

Resolves #77618.

llvmbot · 2024-02-03T13:27:40Z

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Danny Mösch (SimplyDanny)

Changes

Resolves #77618.

Full diff: https://github.com/llvm/llvm-project/pull/80541.diff

8 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
(modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3)
(added) clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp (+77)
(added) clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h (+42)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
(added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst (+44)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp (+48)

diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index 28ca52f46943a..6852db6c2ee31 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -31,6 +31,7 @@ add_clang_library(clangTidyModernizeModule
   UseBoolLiteralsCheck.cpp
   UseConstraintsCheck.cpp
   UseDefaultMemberInitCheck.cpp
+  UseDesignatedInitializersCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index 654f4bd0c6ba4..e96cf274f58cf 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -32,6 +32,7 @@
 #include "UseBoolLiteralsCheck.h"
 #include "UseConstraintsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
+#include "UseDesignatedInitializersCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
@@ -68,6 +69,8 @@ class ModernizeModule : public ClangTidyModule {
     CheckFactories.registerCheck<MakeSharedCheck>("modernize-make-shared");
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
+    CheckFactories.registerCheck<UseDesignatedInitializersCheck>(
+        "modernize-use-designated-initializers");
     CheckFactories.registerCheck<UseStartsEndsWithCheck>(
         "modernize-use-starts-ends-with");
     CheckFactories.registerCheck<UseStdNumbersCheck>(
diff --git a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
new file mode 100644
index 0000000000000..d269cef13e6aa
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
@@ -0,0 +1,77 @@
+//===--- UseDesignatedInitializersCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseDesignatedInitializersCheck.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include <algorithm>
+#include <iterator>
+#include <vector>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::modernize {
+
+static constexpr auto IgnoreSingleElementAggregatesName =
+    "IgnoreSingleElementAggregates";
+static constexpr auto IgnoreSingleElementAggregatesDefault = true;
+
+static std::vector<Stmt *>
+getUndesignatedComponents(const InitListExpr *SyntacticInitList) {
+  std::vector<Stmt *> Result;
+  std::copy_if(SyntacticInitList->begin(), SyntacticInitList->end(),
+               std::back_inserter(Result),
+               [](auto S) { return !isa<DesignatedInitExpr>(S); });
+  return Result;
+}
+
+UseDesignatedInitializersCheck::UseDesignatedInitializersCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreSingleElementAggregates(
+          Options.getLocalOrGlobal(IgnoreSingleElementAggregatesName,
+                                   IgnoreSingleElementAggregatesDefault)) {}
+
+void UseDesignatedInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      initListExpr(hasType(recordDecl().bind("type"))).bind("init"), this);
+}
+
+void UseDesignatedInitializersCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *InitList = Result.Nodes.getNodeAs<InitListExpr>("init");
+  const auto *Type = Result.Nodes.getNodeAs<CXXRecordDecl>("type");
+  if (!Type || !InitList || !Type->isAggregate())
+    return;
+  if (IgnoreSingleElementAggregates && InitList->getNumInits() == 1)
+    return;
+  if (const auto *SyntacticInitList = InitList->getSyntacticForm()) {
+    const auto UndesignatedComponents =
+        getUndesignatedComponents(SyntacticInitList);
+    if (UndesignatedComponents.empty())
+      return;
+    if (UndesignatedComponents.size() == SyntacticInitList->getNumInits()) {
+      diag(InitList->getLBraceLoc(), "use designated initializer list");
+      return;
+    }
+    for (const auto *InitExpr : UndesignatedComponents) {
+      diag(InitExpr->getBeginLoc(), "use designated init expression");
+    }
+  }
+}
+
+void UseDesignatedInitializersCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, IgnoreSingleElementAggregatesName,
+                IgnoreSingleElementAggregates);
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h
new file mode 100644
index 0000000000000..34290aba06fab
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h
@@ -0,0 +1,42 @@
+//===--- UseDesignatedInitializersCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEDESIGNATEDINITIALIZERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEDESIGNATEDINITIALIZERSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::modernize {
+
+/// Triggers on initializer lists for aggregate type that could be
+/// written as designated initializers instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-designated-initializers.html
+class UseDesignatedInitializersCheck : public ClangTidyCheck {
+public:
+  UseDesignatedInitializersCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus20;
+  }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  bool IgnoreSingleElementAggregates;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEDESIGNATEDINITIALIZERSCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9e819ea34c397..ce86f2ea455ff 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,6 +100,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`modernize-use-designated-initializers
+  <clang-tidy/checks/modernize/use-designated-initializers>` check.
+
+  Triggers on initializer lists for aggregate type that could be
+  written as designated initializers instead.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index f40192ed9dea2..5d3b99e0996c2 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -287,6 +287,7 @@ Clang-Tidy Checks
    :doc:`modernize-use-bool-literals <modernize/use-bool-literals>`, "Yes"
    :doc:`modernize-use-constraints <modernize/use-constraints>`, "Yes"
    :doc:`modernize-use-default-member-init <modernize/use-default-member-init>`, "Yes"
+   :doc:`modernize-use-designated-initializers <modernize/use-designated-initializers>`, "Yes"
    :doc:`modernize-use-emplace <modernize/use-emplace>`, "Yes"
    :doc:`modernize-use-equals-default <modernize/use-equals-default>`, "Yes"
    :doc:`modernize-use-equals-delete <modernize/use-equals-delete>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst
new file mode 100644
index 0000000000000..1e182218dbd8e
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst
@@ -0,0 +1,44 @@
+.. title:: clang-tidy - modernize-use-designated-initializers
+
+modernize-use-designated-initializers
+=====================================
+
+Triggers on initializer lists for aggregate type that could be written as
+designated initializers instead.
+
+With plain initializer lists, it is very easy to introduce bugs when adding
+new fields in the middle of a struct or class type. The same confusion might
+arise when changing the order of fields.
+
+C++ 20 supports the designated initializer syntax for aggregate types.
+By applying it, we can always be sure that aggregates are constructed correctly,
+because the every variable being initialized is referenced by name.
+
+Example:
+
+.. code-block::
+
+    struct S { int i, j; };
+
+is an aggregate type that should be initialized as
+
+.. code-block::
+
+    S s{.i = 1, .j = 2};
+
+instead of
+
+.. code-block::
+
+    S s{1, 2};
+
+which could easily become an issue when ``i`` and ``j`` are swapped in the
+declaration of ``S``.
+
+Options
+-------
+
+.. options:: IgnoreSingleElementAggregates
+
+    The value `false` specifies that even initializers for aggregate types
+    with only a single element should be checked. The default value is `true`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp
new file mode 100644
index 0000000000000..d932e2d5e4863
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp
@@ -0,0 +1,48 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-designated-initializers %t
+// RUN: %check_clang_tidy -check-suffixes=,SINGLE-ELEMENT -std=c++20 %s modernize-use-designated-initializers %t \
+// RUN:     -- -config="{CheckOptions: [{key: modernize-use-designated-initializers.IgnoreSingleElementAggregates, value: false}]}" \
+// RUN:     --
+
+struct S1 {};
+
+S1 s11{};
+S1 s12 = {};
+S1 s13();
+S1 s14;
+
+struct S2 { int i, j; };
+
+S2 s21{.i=1, .j =2};
+
+S2 s22 = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use designated initializer list [modernize-use-designated-initializers]
+
+S2 s23{1};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use designated initializer list [modernize-use-designated-initializers]
+
+S2 s24{.i = 1};
+
+S2 s25 = {.i=1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use designated init expression [modernize-use-designated-initializers]
+
+class S3 {
+  public:
+    S2 s2;
+    double d;
+};
+
+S3 s31 = {.s2 = 1, 2, 3.1};
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use designated init expression [modernize-use-designated-initializers]
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: use designated init expression [modernize-use-designated-initializers]
+
+S3 s32 = {{.i = 1, 2}};
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use designated initializer list [modernize-use-designated-initializers]
+// CHECK-MESSAGES: :[[@LINE-2]]:20: warning: use designated init expression [modernize-use-designated-initializers]
+
+struct S4 {
+    double d;
+    private: static int i;
+};
+
+S4 s41 {2.2};
+// CHECK-MESSAGES-SINGLE-ELEMENT: :[[@LINE-1]]:8: warning: use designated initializer list [modernize-use-designated-initializers]

PiotrZSL

Still some work is needed, like auto-fixes, and corner cases and some cleanup, but +- looks fine.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang/include/clang/Tooling/DesignatedInitializers.h

SimplyDanny · 2024-02-11T14:02:24Z

@PiotrZSL: I'm not quite sure if I reproduced all your suggested examples properly. Almost all of them appeared to be working as expected already, so I only added them as test cases.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst

5chmidti

A few general comments and I found a bug.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.h

5chmidti · 2024-02-11T18:18:51Z

clang/include/clang/Tooling/DesignatedInitializers.h

+llvm::DenseMap<clang::SourceLocation, std::string>
+getDesignators(const clang::InitListExpr *Syn);


I'd suggest renaming this function to getUnwrittenDesignators because that is what it does. It does not return existing designators (I know the name comes from the clangd implementation).

Do you have a comment on the location? Is it okay to have it in clang/Tooling?

I don't think it is a good location to have this file here because of what the other files contain. One possibility would be to add this into clang-tools-extra/clang-tidy/utils because clangd links the clang-tidy (clangTidy) in its cmake. Another reviewer should confirm this though.

That's also an option, but i think that clangd link to clang-tidy is configurable.

As I understand it, the base of clang-tidy is always linked into Clangd, but linking in all the checks can be disabled. clangTidyUtils is another library anyway and would need to be linked into Clangd separately if the files are moved to clang-tools-extra/clang-tidy/utils.

I have done so in df24916. Please let me know if this looks correct to you.

@PiotrZSL: One last confirmation please. 🙂

clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

PiotrZSL

Personally I do not see any critical issues, just some nits.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp

5chmidti · 2024-02-14T21:17:59Z

You can stream the InitExpr->getSourceRange() into the diagnostics, which would highlight the specific expression that needs a designated initializer.
E.g.:

use-designated-initializers.cpp.tmp.cpp:33:17: warning: use designated init expression [modernize-use-designated-initializers]
   33 | S2 s25 = {.i=1, 200};
      |                 ^~~
      |                 .j=

instead of

use-designated-initializers.cpp.tmp.cpp:33:17: warning: use designated init expression [modernize-use-designated-initializers]
   33 | S2 s25 = {.i=1, 200};
      |                 ^
      |                 .j=

PiotrZSL

Overall looking fine.
Diagnostic could be still improved.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

PiotrZSL

Few style issues, few performance problems, from functional point of view looks fine.
Fix current issues, and it could land.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/modernize/use-designated-initializers.cpp

PiotrZSL

Overall looking fine, not much to complain.
You may consider putting some one-line comments in line 126, 145 about what next block does.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

SimplyDanny · 2024-02-23T11:13:34Z

I don't get why the Windows tests are failing. 🤔 Can you give me a hint maybe, @PiotrZSL?

PiotrZSL · 2024-02-23T14:00:09Z

@SimplyDanny It's crashing on this construction: "S2 s25 = {.i=1, 2};"

SimplyDanny · 2024-02-24T08:46:00Z

For some reason, the designators on Windows come without the prepended dot. Instead, there is a NULL character in front. Very unclear to me why this is ...

Anyway, the performed normalization seems to fix the inconsistency. The last build was successful.

PiotrZSL · 2024-02-29T20:39:51Z

@SimplyDanny

There is a bug, when run (locally) with:
../.build-llvm-project/bin/clang-tidy --checks="-*,modernize-use-designated-initializers" clang/lib/Basic/Module.cpp --header-filter=".*" --fix

It inserts some random characters into Module.h:

-      return Header{UmbrellaAsWritten, UmbrellaRelativeToRootModuleDirectory,
-                    *Hdr};
+      return Header{.NameAsWritten=UmbrellaAsWritten, .<E1><E2>[<EA>U^@^@<B2>GI<AD><89>}<95><DA>ootModuleDirectory=UmbrellaRelativeToRootModuleDirectory,
+                    .Entry=*Hdr};

Looks like "use after free" error.
Probably that was a reason why tests were toggling.

Valgrind detect this as:

==41359== Invalid read of size 1
==41359==    at 0x6A03309: memmove (vg_replace_strmem.c:1398)
==41359==    by 0x2E039EB: llvm::raw_svector_ostream::write_impl(char const*, unsigned long) (in /fpwork/.build-llvm-project/bin/clang-tidy)
==41359==    by 0x2E01C2F: llvm::raw_ostream::write(char const*, unsigned long) (in /fpwork/.build-llvm-project/bin/clang-tidy)
==41359==    by 0x2E122C1: llvm::Twine::toStringRef(llvm::SmallVectorImpl<char>&) const (in /fpwork/.build-llvm-project/bin/clang-tidy)
==41359==    by 0x2E1214E: llvm::Twine::str[abi:cxx11]() const (in /fpwork/.build-llvm-project/bin/clang-tidy)
==41359==    by 0x3C8B2B7: clang::tidy::modernize::UseDesignatedInitializersCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&)

…tializers Instance of DenseMap were copied into local variable, and as a result reference to string stored in that local variable were returned from function. At the end fix-it were applied with already destroyed string causing some non utf-8 characters to be printed. Related to #80541

SimplyDanny · 2024-03-01T06:59:03Z

Thank you for the fast fix, @PiotrZSL!

sam-mccall · 2024-04-16T15:01:10Z

Hey folks,

I'm glad the clangd code for designators was useful, but this dep from clangd into the middle of tidy::utils isn't really OK - clangd depends on tidy only for the purposes of running tidy checks. It would have been appropriate to have a clangd reviewer on this.

AFAIK the best place we have for code shared between tools is as a new library under Tooling - it's not a perfect fit, but there's precedent for this in Tooling/Inclusions. As a shared library, this would also need unit tests distinct from the integration tests in tidy+clangd.

I'm not sure how best to proceed here - if it's possible to agree soon on a fix forward, let's do that, otherwise I'd plan to revert next week and you can fix as you see fit (extract a shared library, copy the code, etc).

cc @kadircet @HighCommander4

SimplyDanny · 2024-04-16T18:25:29Z

I'm glad the clangd code for designators was useful, but this dep from clangd into the middle of tidy::utils isn't really OK - clangd depends on tidy only for the purposes of running tidy checks. It would have been appropriate to have a clangd reviewer on this.

Why is tidy::utils not okay?

AFAIK the best place we have for code shared between tools is as a new library under Tooling - it's not a perfect fit, but there's precedent for this in Tooling/Inclusions. As a shared library, this would also need unit tests distinct from the integration tests in tidy+clangd.

This is actually the first place where I put the utility. So I'm fine with moving it back there.

I'm not sure how best to proceed here - if it's possible to agree soon on a fix forward, let's do that, otherwise I'd plan to revert next week and you can fix as you see fit (extract a shared library, copy the code, etc).

Why is it such a big deal that a revert needs to be considered?

PiotrZSL · 2024-04-16T20:19:08Z

It would have been appropriate to have a clangd reviewer on this.

That's a more a problem of why group pr-subscribers-clangd were not auto-added.

As a shared library, this would also need unit tests distinct from the integration tests in tidy+clangd.

This were main reason, why it were avoided, as there were no unit tests for this code on clangd side.

I'd plan to revert next week

No reverts on 1.5 month old (clang-tidy) code, specially that this doesn't block anything else.
But fell free to revert clangd part alone. As for library extraction, that can happen in separate pull-request.

sam-mccall · 2024-04-16T22:51:17Z

TL;DR: sounds like I should revert the removals/deps in clangd, and let you decide how to structure clang-tidy's copy of the code?

tidy::utils as a dep has a conceptual layering problem, and a few practical ones: it brings in a pile of dependencies that aren't otherwise in the no-tidy build (effect: much longer build times), and it sets the wrong expectations around performance: that meeting the batch-latency requirements of clang-tidy is sufficient, rather than the interactive-latency requirements of clangd. (Not a theoretical concern: clangd's deliberate deps on clang-tidy have had multiple unacceptable performance regressions in the past, which is the cost of taking the dep, but not one that needs paying here).

This were main reason, why it were avoided, as there were no unit tests for this code on clangd side.

This wasn't a library in its own right on the clangd side; it was part of a feature and tested there. If it's a library with multiple clients, it needs tests so problems with the library can be distinguished from problems with the clients. (I think it would have been nice to have fine-grained unittests anyway, but we didn't).

Why is it such a big deal that a revert needs to be considered?

It's not a big deal, but I thought the most likely fix was to move this into a separate library with tests, I wasn't sure if anyone wanted to take that on in a hurry, and wanted to get back to a good state.

If tidy owners are happier with just cloning the code, I can revert just the changes to clangd.

I would suggest moving the code out out tidy/utils and into the check if that's the only place it's going to be tested, but that's up to you.

llvmbot added clang-tools-extra clang-tidy labels Feb 3, 2024

PiotrZSL requested changes Feb 3, 2024

View reviewed changes

EugeneZelenko reviewed Feb 3, 2024

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp Outdated Show resolved Hide resolved

SimplyDanny force-pushed the use-designated-initializer branch from 68de71c to c2bb32a Compare February 11, 2024 13:47

SimplyDanny commented Feb 11, 2024

View reviewed changes

clang/include/clang/Tooling/DesignatedInitializers.h Outdated Show resolved Hide resolved

EugeneZelenko reviewed Feb 11, 2024

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/modernize/use-designated-initializers.rst Outdated Show resolved Hide resolved

5chmidti requested changes Feb 11, 2024

View reviewed changes

SimplyDanny requested a review from PiotrZSL February 13, 2024 20:20

EugeneZelenko reviewed Feb 13, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

PiotrZSL reviewed Feb 13, 2024

View reviewed changes

PiotrZSL approved these changes Feb 14, 2024

View reviewed changes

SimplyDanny force-pushed the use-designated-initializer branch 3 times, most recently from 64aebae to 0e4bae4 Compare February 17, 2024 18:38

SimplyDanny requested a review from 5chmidti February 19, 2024 09:09

PiotrZSL requested changes Feb 20, 2024

View reviewed changes

SimplyDanny force-pushed the use-designated-initializer branch from df24916 to 14c991c Compare February 20, 2024 20:28

PiotrZSL approved these changes Feb 20, 2024

View reviewed changes

SimplyDanny force-pushed the use-designated-initializer branch from a3b8c5b to e50677a Compare February 22, 2024 08:02

SimplyDanny force-pushed the use-designated-initializer branch from 12d455a to e50677a Compare February 23, 2024 11:13

SimplyDanny force-pushed the use-designated-initializer branch 2 times, most recently from 3768820 to 07fb7ad Compare February 23, 2024 21:45

SimplyDanny force-pushed the use-designated-initializer branch from 07fb7ad to 0a633d0 Compare February 24, 2024 17:13

SimplyDanny added 18 commits February 29, 2024 17:58

Compute designators lazily and exit/continue early

fea9ac2

Point to type to initialize

d0e686e

Mention type or field name in diagnostic

c719b05

Move designator computation to clang-tidy/utils and link it into Clangd

a1b17e0

Return without block

e97bb92

Have safety check for both cases

b49e587

Prefer references to avoid performance issues

54198fc

Refer to type only

6432616

Improve type hint

5fc2d3d

Use new option format

5081ecb

Extract to class to handle missing elements gracefully

3cfce27

Rename and avoid two calls

906b338

Combine diagnostic details aggregation

6cbf8c4

Add some doc

cb270ff

Make intention clearer

7054558

Omit placeholder if there is no designator

f83c0a6

Trim NULL characters

12f1abf

Fix, format and reorder documentation

4d6446c

SimplyDanny force-pushed the use-designated-initializer branch from 42885d6 to 4d6446c Compare February 29, 2024 17:09

SimplyDanny merged commit 525fe44 into llvm:main Feb 29, 2024

SimplyDanny deleted the use-designated-initializer branch February 29, 2024 19:48

SimplyDanny mentioned this pull request Mar 3, 2024

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

Closed

		llvm::DenseMap<clang::SourceLocation, std::string>
		getDesignators(const clang::InitListExpr *Syn);

[clang-tidy] Add new check modernize-use-designated-initializers #80541

[clang-tidy] Add new check modernize-use-designated-initializers #80541

Uh oh!

Conversation

SimplyDanny commented Feb 3, 2024

Uh oh!

llvmbot commented Feb 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SimplyDanny commented Feb 11, 2024

Uh oh!

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

5chmidti Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

SimplyDanny Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

5chmidti Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

PiotrZSL Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

SimplyDanny Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

SimplyDanny Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

5chmidti commented Feb 14, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[clang-tidy] Add new check `modernize-use-designated-initializers` #80541

[clang-tidy] Add new check `modernize-use-designated-initializers` #80541

llvmbot commented Feb 3, 2024 •

edited

Loading

sam-mccall commented Apr 16, 2024 •

edited

Loading

sam-mccall commented Apr 16, 2024 •

edited

Loading