Inline operator== and operator!=

[ktf]

Avoid triggering -Wnon-template-friend on newer GCC.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Changes

Avoid triggering -Wnon-template-friend on newer GCC.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/ADT/PagedVector.h (+19-24)

diff --git a/llvm/include/llvm/ADT/PagedVector.h b/llvm/include/llvm/ADT/PagedVector.h
index 667bece6d718385..c96e0b20e2d33d7 100644
--- a/llvm/include/llvm/ADT/PagedVector.h
+++ b/llvm/include/llvm/ADT/PagedVector.h
@@ -209,36 +209,31 @@ template <typename T, size_t PageSize = 1024 / sizeof(T)> class PagedVector {
       return PagePtr[ElementIdx % PageSize];
     }
 
+    /// Equality operator.
     friend bool operator==(MaterializedIterator const &LHS,
-                           MaterializedIterator const &RHS);
+                           MaterializedIterator const &RHS) {
+      assert(LHS.PV == RHS.PV);
+      // Make sure we are comparing either end iterators or iterators pointing
+      // to materialized elements.
+      // It should not be possible to build two iterators pointing to non
+      // materialized elements.
+      assert(LHS.ElementIdx == LHS.PV->Size ||
+             (LHS.ElementIdx < LHS.PV->Size &&
+              LHS.PV->PageToDataPtrs[LHS.ElementIdx / PageSize]));
+      assert(RHS.ElementIdx == RHS.PV->Size ||
+             (RHS.ElementIdx < RHS.PV->Size &&
+              RHS.PV->PageToDataPtrs[RHS.ElementIdx / PageSize]));
+      return LHS.ElementIdx == RHS.ElementIdx;
+    }
+
     friend bool operator!=(MaterializedIterator const &LHS,
-                           MaterializedIterator const &RHS);
+                           MaterializedIterator const &RHS) {
+      return !(LHS == RHS);
+    }
 
     [[nodiscard]] size_t getIndex() const { return ElementIdx; }
   };
 
-  /// Equality operator.
-  friend bool operator==(MaterializedIterator const &LHS,
-                         MaterializedIterator const &RHS) {
-    assert(LHS.PV == RHS.PV);
-    // Make sure we are comparing either end iterators or iterators pointing
-    // to materialized elements.
-    // It should not be possible to build two iterators pointing to non
-    // materialized elements.
-    assert(LHS.ElementIdx == LHS.PV->Size ||
-           (LHS.ElementIdx < LHS.PV->Size &&
-            LHS.PV->PageToDataPtrs[LHS.ElementIdx / PageSize]));
-    assert(RHS.ElementIdx == RHS.PV->Size ||
-           (RHS.ElementIdx < RHS.PV->Size &&
-            RHS.PV->PageToDataPtrs[RHS.ElementIdx / PageSize]));
-    return LHS.ElementIdx == RHS.ElementIdx;
-  }
-
-  friend bool operator!=(MaterializedIterator const &LHS,
-                         MaterializedIterator const &RHS) {
-    return !(LHS == RHS);
-  }
-
   /// Iterators over the materialized elements of the vector.
   ///
   /// This includes all the elements belonging to allocated pages,

[dwblaikie]

If it does what it says on the tin, sounds good to me.

[ktf]

It does and it breaks windows builds.

[dyung]

Hi, we also use gcc internally to build the compiler and were seeing the warnings you are trying to fix here, so I tried applying your fix on our repo and it seems to fail with gcc 9.4.0 as well for the same errors I'm seeing with my Windows build of your fix. Namely this error:

/home/dyung/src/git/merge/llvm/include/llvm/ADT/PagedVector.h:220:40: error: ‘size_t llvm::PagedVector<clang::QualType>::Size’ is private within this context
  220 |       assert(LHS.ElementIdx == LHS.PV->Size ||
      |                                ~~~~~~~~^~~~
In file included from /home/dyung/src/git/merge/clang/include/clang/Basic/SourceManager.h:46,
                 from /home/dyung/src/git/merge/clang/lib/Serialization/ASTReader.cpp:61:
/home/dyung/src/git/merge/llvm/include/llvm/ADT/PagedVector.h:46:10: note: declared private here
   46 |   size_t Size = 0;
      |          ^~~~
...
/home/dyung/src/git/merge/llvm/include/llvm/ADT/PagedVector.h:222:23: error: ‘llvm::SmallVector<clang::QualType*, 0> llvm::PagedVector<clang::QualType>::PageToDataPtrs’ is private within this context
  222 |               LHS.PV->PageToDataPtrs[LHS.ElementIdx / PageSize]));
      |               ~~~~~~~~^~~~~~~~~~~~~~
In file included from /home/dyung/src/git/merge/clang/include/clang/Basic/SourceManager.h:46,
                 from /home/dyung/src/git/merge/clang/lib/Serialization/ASTReader.cpp:61:
/home/dyung/src/git/merge/llvm/include/llvm/ADT/PagedVector.h:50:31: note: declared private here
   50 |   mutable SmallVector<T *, 0> PageToDataPtrs;
      |                               ^~~~~~~~~~~~~~

[kuhar]

So it would seem we would have to make the operator== friend of both the vector and the iterator class? Something like:

template <...>
class PagedVector {
  ...

  class MaterializedIterator;
  friend bool operator==(const MaterializeIterator& lhs, const MaterializedIterator& rhs);

  class MaterializedIterator {
    friend bool operator==(const MaterializeIterator& lhs, const MaterializedIterator& rhs) {
      ...
    }
  };
};

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ktf]

Using @zygoloid suggestion it works on GCC (including 9.4, as tested on godbolt), macOS XCode 15, now waiting for the CI to verify Windows.

@kuhar my understanding of your suggestion still triggers the -Wnon-template-friend warning on GCC.

Side note: does anyone know how to get LLVM work in godbolt? Using the vspkg package does not work for me (missing includes from llvm/.*).

[ktf]

I am waiting for Windows to finish to reformat the PR...

[ktf]

Ping @kuhar @zygoloid @dwblaikie. This seems to work without warnings on all the compilers I managed to test (gcc 9.4.0 on godbolt, latest GCC on godbolt, macOS XCode 15, and of course the CI). Adapted from @zygoloid suggestion.

[mikaelholmen]

Hi! Anything preventing pushing this now?

[ktf]

I was wondering the same. I do not have write rights.

[mikaelholmen]

I was wondering the same. I do not have write rights.

If you got your patch approved and don't have commit rights, you need to say that so someone else can push it for you. :)

[ktf]

@vgvassilev could you take care of this?