Skip to content

[ADT] Remove a constructor (NFC) #146010

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

Merged

Conversation

kazutakahirata
Copy link
Contributor

ArrayRef now has a new constructor that takes a parameter whose type
has data() and size() methods. Since the new constructor subsumes
another constructor that takes std::array, this patch removes that
constructor. Note that std::array also comes with data() and size()
methods.

The only problem is that ASTFileSignature in the clang frontend does
not work with the new ArrayRef constructor because it overrides size,
blocking access to std::array<uint8_t, 20>::size(). This patch adds
an implicit cast operator to ArrayRef. Note that ASTFileSignature is
defined as:

struct ASTFileSignature : std::array<uint8_t, 20> {
using BaseT = std::array<uint8_t, 20>;
static constexpr size_t size = std::tuple_size::value;
:

ArrayRef now has a new constructor that takes a parameter whose type
has data() and size() methods.  Since the new constructor subsumes
another constructor that takes std::array, this patch removes that
constructor.  Note that std::array also comes with data() and size()
methods.

The only problem is that ASTFileSignature in the clang frontend does
not work with the new ArrayRef constructor because it overrides size,
blocking access to std::array<uint8_t, 20>::size().  This patch adds
an implicit cast operator to ArrayRef.  Note that ASTFileSignature is
defined as:

  struct ASTFileSignature : std::array<uint8_t, 20> {
    using BaseT = std::array<uint8_t, 20>;
    static constexpr size_t size = std::tuple_size<BaseT>::value;
    :
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules llvm:adt labels Jun 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-clang-modules

Author: Kazu Hirata (kazutakahirata)

Changes

ArrayRef now has a new constructor that takes a parameter whose type
has data() and size() methods. Since the new constructor subsumes
another constructor that takes std::array, this patch removes that
constructor. Note that std::array also comes with data() and size()
methods.

The only problem is that ASTFileSignature in the clang frontend does
not work with the new ArrayRef constructor because it overrides size,
blocking access to std::array<uint8_t, 20>::size(). This patch adds
an implicit cast operator to ArrayRef. Note that ASTFileSignature is
defined as:

struct ASTFileSignature : std::array<uint8_t, 20> {
using BaseT = std::array<uint8_t, 20>;
static constexpr size_t size = std::tuple_size<BaseT>::value;
:


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/Module.h (+5)
  • (modified) llvm/include/llvm/ADT/ArrayRef.h (-5)
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 3d035f0a5f787..69a1de6f79b35 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -64,6 +64,11 @@ struct ASTFileSignature : std::array<uint8_t, 20> {
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  // Support implicit cast to ArrayRef.  Note that ASTFileSignature::size
+  // prevents implicit cast to ArrayRef because one of the implicit constructors
+  // of ArrayRef requires access to BaseT::size.
+  operator ArrayRef<uint8_t>() const { return ArrayRef<uint8_t>(data(), size); }
+
   /// Returns the value truncated to the size of an uint64_t.
   uint64_t truncatedValue() const {
     uint64_t Value = 0;
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index 892482d64e4a1..268510b803ea0 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -98,11 +98,6 @@ namespace llvm {
     /*implicit*/ constexpr ArrayRef(const C &V)
         : Data(V.data()), Length(V.size()) {}
 
-    /// Construct an ArrayRef from a std::array
-    template <size_t N>
-    /*implicit*/ constexpr ArrayRef(const std::array<T, N> &Arr)
-        : Data(Arr.data()), Length(N) {}
-
     /// Construct an ArrayRef from a C array.
     template <size_t N>
     /*implicit*/ constexpr ArrayRef(const T (&Arr LLVM_LIFETIME_BOUND)[N])

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-clang

Author: Kazu Hirata (kazutakahirata)

Changes

ArrayRef now has a new constructor that takes a parameter whose type
has data() and size() methods. Since the new constructor subsumes
another constructor that takes std::array, this patch removes that
constructor. Note that std::array also comes with data() and size()
methods.

The only problem is that ASTFileSignature in the clang frontend does
not work with the new ArrayRef constructor because it overrides size,
blocking access to std::array<uint8_t, 20>::size(). This patch adds
an implicit cast operator to ArrayRef. Note that ASTFileSignature is
defined as:

struct ASTFileSignature : std::array<uint8_t, 20> {
using BaseT = std::array<uint8_t, 20>;
static constexpr size_t size = std::tuple_size<BaseT>::value;
:


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/Module.h (+5)
  • (modified) llvm/include/llvm/ADT/ArrayRef.h (-5)
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 3d035f0a5f787..69a1de6f79b35 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -64,6 +64,11 @@ struct ASTFileSignature : std::array<uint8_t, 20> {
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  // Support implicit cast to ArrayRef.  Note that ASTFileSignature::size
+  // prevents implicit cast to ArrayRef because one of the implicit constructors
+  // of ArrayRef requires access to BaseT::size.
+  operator ArrayRef<uint8_t>() const { return ArrayRef<uint8_t>(data(), size); }
+
   /// Returns the value truncated to the size of an uint64_t.
   uint64_t truncatedValue() const {
     uint64_t Value = 0;
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index 892482d64e4a1..268510b803ea0 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -98,11 +98,6 @@ namespace llvm {
     /*implicit*/ constexpr ArrayRef(const C &V)
         : Data(V.data()), Length(V.size()) {}
 
-    /// Construct an ArrayRef from a std::array
-    template <size_t N>
-    /*implicit*/ constexpr ArrayRef(const std::array<T, N> &Arr)
-        : Data(Arr.data()), Length(N) {}
-
     /// Construct an ArrayRef from a C array.
     template <size_t N>
     /*implicit*/ constexpr ArrayRef(const T (&Arr LLVM_LIFETIME_BOUND)[N])

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

ArrayRef now has a new constructor that takes a parameter whose type
has data() and size() methods. Since the new constructor subsumes
another constructor that takes std::array, this patch removes that
constructor. Note that std::array also comes with data() and size()
methods.

The only problem is that ASTFileSignature in the clang frontend does
not work with the new ArrayRef constructor because it overrides size,
blocking access to std::array<uint8_t, 20>::size(). This patch adds
an implicit cast operator to ArrayRef. Note that ASTFileSignature is
defined as:

struct ASTFileSignature : std::array<uint8_t, 20> {
using BaseT = std::array<uint8_t, 20>;
static constexpr size_t size = std::tuple_size<BaseT>::value;
:


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/Module.h (+5)
  • (modified) llvm/include/llvm/ADT/ArrayRef.h (-5)
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 3d035f0a5f787..69a1de6f79b35 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -64,6 +64,11 @@ struct ASTFileSignature : std::array<uint8_t, 20> {
 
   explicit operator bool() const { return *this != BaseT({{0}}); }
 
+  // Support implicit cast to ArrayRef.  Note that ASTFileSignature::size
+  // prevents implicit cast to ArrayRef because one of the implicit constructors
+  // of ArrayRef requires access to BaseT::size.
+  operator ArrayRef<uint8_t>() const { return ArrayRef<uint8_t>(data(), size); }
+
   /// Returns the value truncated to the size of an uint64_t.
   uint64_t truncatedValue() const {
     uint64_t Value = 0;
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index 892482d64e4a1..268510b803ea0 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -98,11 +98,6 @@ namespace llvm {
     /*implicit*/ constexpr ArrayRef(const C &V)
         : Data(V.data()), Length(V.size()) {}
 
-    /// Construct an ArrayRef from a std::array
-    template <size_t N>
-    /*implicit*/ constexpr ArrayRef(const std::array<T, N> &Arr)
-        : Data(Arr.data()), Length(N) {}
-
     /// Construct an ArrayRef from a C array.
     template <size_t N>
     /*implicit*/ constexpr ArrayRef(const T (&Arr LLVM_LIFETIME_BOUND)[N])

Copy link
Member

@d0k d0k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same constructor also exists on MutableArrayRef

@kazutakahirata
Copy link
Contributor Author

The same constructor also exists on MutableArrayRef

@d0k I'll get to that in a separate PR. Thanks for the review!

@kazutakahirata kazutakahirata merged commit f0311f4 into llvm:main Jun 27, 2025
12 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20250626_ArrayRef_std_array branch June 27, 2025 14:39
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jun 27, 2025

LLVM Buildbot has detected a new failure on builder premerge-monolithic-linux running on premerge-linux-1 while building clang,llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/36246

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: tools/llvm-exegesis/RISCV/rvv/filter.test' FAILED ********************
Exit Code: 2

Command Output (stderr):
--
/build/buildbot/premerge-monolithic-linux/build/bin/llvm-exegesis -mtriple=riscv64 -mcpu=sifive-x280 -benchmark-phase=assemble-measured-code --mode=inverse_throughput --opcode-name=PseudoVNCLIPU_WX_M1_MASK     --riscv-filter-config='vtype = {VXRM: rod, AVL: VLMAX, SEW: e(8|16), Policy: ta/mu}' --max-configs-per-opcode=1000 --min-instructions=10 | /build/buildbot/premerge-monolithic-linux/build/bin/FileCheck /build/buildbot/premerge-monolithic-linux/llvm-project/llvm/test/tools/llvm-exegesis/RISCV/rvv/filter.test # RUN: at line 1
+ /build/buildbot/premerge-monolithic-linux/build/bin/llvm-exegesis -mtriple=riscv64 -mcpu=sifive-x280 -benchmark-phase=assemble-measured-code --mode=inverse_throughput --opcode-name=PseudoVNCLIPU_WX_M1_MASK '--riscv-filter-config=vtype = {VXRM: rod, AVL: VLMAX, SEW: e(8|16), Policy: ta/mu}' --max-configs-per-opcode=1000 --min-instructions=10
+ /build/buildbot/premerge-monolithic-linux/build/bin/FileCheck /build/buildbot/premerge-monolithic-linux/llvm-project/llvm/test/tools/llvm-exegesis/RISCV/rvv/filter.test
PseudoVNCLIPU_WX_M1_MASK: Failed to produce any snippet via: instruction has tied variables, avoiding Read-After-Write issue, picking random def and use registers not aliasing each other, for uses, one unique register for each position
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /build/buildbot/premerge-monolithic-linux/build/bin/FileCheck /build/buildbot/premerge-monolithic-linux/llvm-project/llvm/test/tools/llvm-exegesis/RISCV/rvv/filter.test

--

********************


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category llvm:adt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants