ronantakizawa/codereview-bench huggingface dataset:
Repository: llvm/llvm-project
File: libsycl/src/detail/context_impl.hpp (C++)

Code before:
/// backends where context exists and participates in operations liboffload
/// plugins create and use default context that represents all devices in that
/// platform. Duplicating this logic here.
class ContextImpl : public std::enable_shared_from_this<ContextImpl> {
  struct Private {
    explicit Private() = default;
  };

public:
  /// Constructs a ContextImpl using a platform.
  ///
  /// Newly created instance represents all devices in platform.
  ///
  /// \param Platform is a platform to associate this context with.
  ContextImpl(PlatformImpl &Platform, Private) : MPlatform(Platform) {}

  /// Constructs a ContextImpl with a provided arguments. Variadic helper.
  /// Restrics ways of ContextImpl creation.
  template <typename... Ts>
  static std::shared_ptr<ContextImpl> create(Ts &&...args) {
    return std::make_shared<ContextImpl>(std::forward<Ts>(args)..., Private{});
  }

  /// Returns associated platform
  ///
  /// \return platform implementation object this context is associated with.
  PlatformImpl &getPlatformImpl() const { return MPlatform; }

  /// Calls "callback" with every device associated
  /// with this context.
  void iterateDevices(const std::function<void(DeviceImpl *)> &callback) const;

  /// Returns backend of the platform this context is associated with.
  ///
  /// \return SYCL backend.
  backend getBackend() const;

private:
  PlatformImpl &MPlatform;
};

} // namespace detail

_LIBSYCL_END_NAMESPACE_SYCL

#endif // _LIBSYCL_CONTEXT_IMPL


Review comment (suggestion):
```suggestion
  /// \return platform implementation object this context is associated with.
```

Code after:
/// Context represents the runtime data structures and state required by a SYCL
/// backend API to interact with a group of devices associated with a platform.
class ContextImpl : public std::enable_shared_from_this<ContextImpl> {
  struct Private {
    explicit Private() = default;
  };

public:
  /// Constructs a ContextImpl using a platform.
  ///
  /// Newly created instance represents all devices in platform.
  ///
  /// \param Platform is a platform to associate this context with.
  ContextImpl(PlatformImpl &Platform, Private) : MPlatform(Platform) {}

  /// Constructs a ContextImpl with a provided arguments. Variadic helper.
  /// Restrics ways of ContextImpl creation.
  template <typename... Ts>
  static std::shared_ptr<ContextImpl> create(Ts &&...args) {
    return std::make_shared<ContextImpl>(std::forward<Ts>(args)..., Private{});
  }

  /// \return the platform this context is associated with.
  PlatformImpl &getPlatformImpl() const { return MPlatform; }

  /// Calls "callback" with every device associated
  /// with this context.
  void iterateDevices(const std::function<void(DeviceImpl *)> &callback) const;

  /// \return backend of the platform this context is associated with.
  backend getBackend() const;

private:
  PlatformImpl &MPlatform;
};

} // namespace detail

_LIBSYCL_END_NAMESPACE_SYCL

#endif // _LIBSYCL_CONTEXT_IMPL
Repository: llvm/llvm-project
File: libc/test/src/__support/FPUtil/bfloat16_test.cpp (C++)

Code before:
    BFloat16 bf16_num{bits};
    MPFRNumber mpfr_num{bf16_num};

    // bfloat16 to float
    float mpfr_float = mpfr_num.as<float>();
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_float, static_cast<float>(bf16_num));

    // float to bfloat16
    BFloat16 bf16_from_float{mpfr_float};
    MPFRNumber mpfr_num_2{mpfr_float};
    BFloat16 mpfr_bfloat = mpfr_num_2.as<BFloat16>();
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, bf16_from_float);
  }
}

TEST_F(LlvmLibcBfloat16ConversionTest, FromInteger) {
  constexpr int RANGE = 100'000;
  for (int i = -RANGE; i <= RANGE; i++) {
    BFloat16 mpfr_bfloat = MPFRNumber(i).as<BFloat16>();
    BFloat16 libc_bfloat{i};
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, libc_bfloat);
  }
}

TEST_F(LlvmLibcBfloat16ConversionTest, MultiplyAssign) {
  for (uint16_t i = POS_START; i <= POS_STOP; i++) {
    for (uint16_t j = POS_START; j <= POS_STOP; j++) {
      BFloat16 a{i}, b{j};
      MPFRNumber mpfr_a{a}, mpfr_b{b};
      MPFRNumber mpfr_c = mpfr_a;
      mpfr_c.mul(mpfr_b);
      BFloat16 mpfr_bfloat = mpfr_c.as<BFloat16>();
      a *= b;
      BFloat16 libc_bfloat = a;
      EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, libc_bfloat);
    }
  }
}


Review comment (suggestion):
we don't need extensive tests for this.  Multiplication test should cover it already.  Just need some smoke tests to make sure `a *= b;` works correctly.

Code after:
    BFloat16 bf16_num{bits};
    MPFRNumber mpfr_num{bf16_num};

    // bfloat16 to float
    float mpfr_float = mpfr_num.as<float>();
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_float, static_cast<float>(bf16_num));

    // float to bfloat16
    BFloat16 bf16_from_float{mpfr_float};
    MPFRNumber mpfr_num_2{mpfr_float};
    BFloat16 mpfr_bfloat = mpfr_num_2.as<BFloat16>();
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, bf16_from_float);
  }
}

TEST_F(LlvmLibcBfloat16ConversionTest, FromInteger) {
  constexpr int RANGE = 100'000;
  for (int i = -RANGE; i <= RANGE; i++) {
    BFloat16 mpfr_bfloat = MPFRNumber(i).as<BFloat16>();
    BFloat16 libc_bfloat{i};
    EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, libc_bfloat);
  }
}

TEST_F(LlvmLibcBfloat16ConversionTest, MultiplyAssign) {

  constexpr BFloat16 VAL[] = {zero,           neg_zero,        inf,
                              neg_inf,        min_normal,      max_normal,
                              bfloat16(1.0f), bfloat16(-1.0f), bfloat16(2.0f),
                              bfloat16(3.0f)};
  for (const bfloat16 &x : VAL) {
    for (const bfloat16 &y : VAL) {
      BFloat16 a = x, b = y;
      MPFRNumber mpfr_a{a}, mpfr_b{b};
      MPFRNumber mpfr_c = mpfr_a.mul(mpfr_b);
      BFloat16 mpfr_bfloat = mpfr_c.as<BFloat16>();
      a *= b;
      BFloat16 libc_bfloat = a;
      EXPECT_FP_EQ_ALL_ROUNDING(mpfr_bfloat, libc_bfloat);
    }
  }
}
Repository: llvm/llvm-project
File: clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp (C++)

Code before:
                                         // Removal
                                         "extract", "erase", "clear",
                                         // Assignment
                                         "swap", "replace"};

  return llvm::StringSwitch<const llvm::StringSet<> *>(ContainerName)
      .Case("vector", &Vector)
      .Case("basic_string", &String)
      .Case("deque", &Deque)
      .Case("priority_queue", &PriorityQueue)
      .Cases({"set", "multiset", "map", "multimap"}, &Associative)
      .Cases({"unordered_set", "unordered_multiset", "unordered_map",
              "unordered_multimap"},
             &UnorderedAssociative)
      .Cases({"flat_map", "flat_set", "flat_multimap", "flat_multiset"}, &Flat)
      .Default(nullptr);
}

bool isContainerInvalidationMethod(const CXXMethodDecl &MD) {
  const CXXRecordDecl *RD = MD.getParent();
  if (!isInStlNamespace(RD))
    return false;

  StringRef ContainerName = getName(*RD);
  const llvm::StringSet<> *InvalidatingMethods =
      getInvalidatingMethods(ContainerName);
  if (!InvalidatingMethods)
    return false;

  // Handle Operators via OverloadedOperatorKind
  OverloadedOperatorKind OO = MD.getOverloadedOperator();
  if (OO != OO_None) {
    switch (OO) {
    case OO_Equal:     // operator= : Always invalidates (Assignment)
    case OO_PlusEqual: // operator+= : Append (String/Vector)
      return true;
    case OO_Subscript: // operator[] : Invalidation only for Maps
                       // (Insert-or-access)
    {
      static const llvm::StringSet<> MapContainers = {"map", "unordered_map",
                                                      "flat_map"};
      return MapContainers.contains(ContainerName);
    }
    default:
      return false;
    }
  }

  if (!MD.getIdentifier())
    return false;


Review comment (refactor):
This seems like a bit of leaky abstraction.
Could we just pass both containerName and methodName to `isInvalidatingMethod` and let that function determine if it is invalidating or not.
WDYT?

Code after:
                 &NodeBased)
          .Cases({"flat_map", "flat_set", "flat_multimap", "flat_multiset"},
                 &Flat)
          .Default(nullptr);

  if (!InvalidatingMethods)
    return false;

  // Handle Operators via OverloadedOperatorKind
  OverloadedOperatorKind OO = MD.getOverloadedOperator();
  if (OO != OO_None) {
    switch (OO) {
    case OO_Equal:     // operator= : Always invalidates (Assignment)
    case OO_PlusEqual: // operator+= : Append (String/Vector)
      return true;
    case OO_Subscript: // operator[] : Invalidation only for
                       // `flat_map` (Insert-or-access).
                       // `map` and `unordered_map` are excluded.
      return ContainerName == "flat_map";
    default:
      return false;
    }
  }

  if (!MD.getIdentifier())
    return false;

  return InvalidatingMethods->contains(MD.getName());
}
} // namespace clang::lifetimes
Repository: llvm/llvm-project
File: clang/test/Sema/warn-lifetime-safety-invalidations.cpp (C++)

Code before:
void SetExtractDoesNotInvalidateOthers() {
  std::set<int> s;
  s.insert(1);
  s.insert(2);
  auto it1 = s.begin();
  auto it2 = it1;
  ++it2;
  s.extract(it2);
  *it1;
}

void SetClearInvalidates() {
  std::set<int> s;
  auto it = s.begin(); // expected-warning {{object whose reference is captured is later invalidated}}
  s.clear(); // expected-note {{invalidated here}}
  *it; // expected-note {{later used here}}
}

void MapClearInvalidates() {
  std::map<int, int> m;
  auto it = m.begin();  // expected-warning {{object whose reference is captured is later invalidated}}
  m.clear(); // expected-note {{invalidated here}}
  *it; // expected-note {{later used here}}
}

void MapSubscriptDoesNotInvalidate() {
  std::map<int, int> m;
  auto it = m.begin();
  m[1];
  *it;
}

} // namespace AssociativeContainers


Review comment (suggestion):
Can you also add a case where you use this in a function call.

```cpp
#include <map>
#include <flat_map>

void PrintMax(const int& a, const int& b);

void foo(std::map<int, int> mp, int a, int b) {
    PrintMax(mp[a], mp[b]);
}

void bar(std::flat_map<int, int> mp, int a, int b) {
    PrintMax(mp[a], mp[b]);
}

```

https://godbolt.org/z/5xTec85Wz

Currently both are errors. Expected: first is ok, second is not.

Code after:
void SetExtractDoesNotInvalidateOthers() {
  std::set<int> s;
  s.insert(1);
  s.insert(2);
  auto it1 = s.begin();
  auto it2 = it1;
  ++it2;
  s.extract(it2);
  *it1;
}

void SetClearInvalidates() {
  std::set<int> s;
  auto it = s.begin(); // expected-warning {{object whose reference is captured is later invalidated}}
  s.clear(); // expected-note {{invalidated here}}
  *it; // expected-note {{later used here}}
}

void MapClearInvalidates() {
  std::map<int, int> m;
  auto it = m.begin();  // expected-warning {{object whose reference is captured is later invalidated}}
  m.clear(); // expected-note {{invalidated here}}
  *it; // expected-note {{later used here}}
}

void MapSubscriptDoesNotInvalidate() {
  std::map<int, int> m;
  auto it = m.begin();
  m[1];
  *it;
}

void PrintMax(const int& a, const int& b);

void MapSubscriptMultipleCallsDoesNotInvalidate(std::map<int, int> mp, int a, int b) {
    PrintMax(mp[a], mp[b]);
}

void FlatMapSubscriptMultipleCallsInvalidate(std::flat_map<int, int> mp, int a, int b) {
    PrintMax(mp[a], mp[b]); // expected-warning {{object whose reference is captured is later invalidated}} \
                                 // expected-note {{invalidated here}} \
                                 // expected-note {{later used here}}
}

} // namespace AssociativeContainers
Repository: llvm/llvm-project
File: clang/test/SemaTemplate/concepts.cpp (C++)

Code before:
  quantity_point<default_point_origin(R{})> operator0();
};
template <auto R> point_<decltype(R)> point;
struct absolute_zero : absolute_point_origin<thermodynamic_temperature> {
} absolute_zero;
auto zeroth_kelvin = absolute_zero;
struct : named_unit<"", thermodynamic_temperature, zeroth_kelvin> {
} kelvin;
struct ice_point : relative_point_origin<point<kelvin>> {};

}

namespace GH174667 {

template<class T, class, class U>
concept C = requires{ requires U(T(1)); };

template<C<void, bool> T> int f();
void main() { f<int>(); }

}

namespace GH176402 {
  void f() {
    auto recursiveLambda = [](auto self, int depth) -> void {
      struct MyClass;
      auto testConcept = []<typename T> {
        return requires(T) { &MyClass::operator0 } /* expected-error {{expected ';' at end of requirement}} */;
      };
    };
    recursiveLambda(recursiveLambda, 5);
  }
}


Review comment (bug):
I don't think this test actually exercises the code change. You'd need a test where the alias template canonicalizes from an instantiation-dependent type to a non-dependent type, such as:

```suggestion
struct Bar;

template <typename T> using Foo = Bar;
```

like in the original bug report.

Code after:
  quantity_point<default_point_origin(R{})> operator0();
};
template <auto R> point_<decltype(R)> point;
struct absolute_zero : absolute_point_origin<thermodynamic_temperature> {
} absolute_zero;
auto zeroth_kelvin = absolute_zero;
struct : named_unit<"", thermodynamic_temperature, zeroth_kelvin> {
} kelvin;
struct ice_point : relative_point_origin<point<kelvin>> {};

}

namespace GH182344 {

template <typename T>
  requires true
void f() {}

template <typename T>
  requires false
void f() = delete;

template <typename T>
struct Bar {};

template <typename T> using Foo = Bar<T>;

template <typename T> void use() {
  f<Foo<T>>();
}

}

namespace GH174667 {

template<class T, class, class U>
concept C = requires{ requires U(T(1)); };

template<C<void, bool> T> int f();
void main() { f<int>(); }

}

namespace GH176402 {
  void f() {
    auto recursiveLambda = [](auto self, int depth) -> void {
      struct MyClass;
      auto testConcept = []<typename T> {
        return requires(T) { &MyClass::operator0 } /* expected-error {{expected ';' at end of requirement}} */;
      };
    };
Repository: llvm/llvm-project
File: libcxx/test/libcxx/containers/associative/lower_upper_bound_non_strict_weak_order.pass.cpp (C++)

Code before:
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//

// <set>
// <map>

// This test ensures that libc++ maintains its historical behavior when std::set
// or std::map are used with a comparator that isn't quite a strict weak ordering
// when _LIBCPP_ENABLE_LEGACY_TREE_SEARCH_ALGORITHM is defined.
//
// This escape hatch is only supported as a temporary means to give a bit more time
// for codebases to fix incorrect usages of associative containers.

// This precise test reproduces a comparator similar to boost::icl::exclusive_less
// for discrete right-open intervals: it orders disjoint intervals but treats
// overlapping intervals as equivalent, which is incorrect but was relied upon.

// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_LEGACY_TREE_SEARCH_ALGORITHM

#include <cassert>
#include <map>

Review comment (security):
This test, along with the escape hatch, can be removed in the LLVM 23 time frame. I want to provide an escape hatch since Boost.ICL is pretty widely used and I expect that a lot of people might bang their head against this problem.

Code after:
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//

// <set>
// <map>

// This test ensures that libc++ maintains its historical behavior when std::set
// or std::map are used with a comparator that isn't quite a strict weak ordering
// when _LIBCPP_ENABLE_LEGACY_TREE_LOWER_UPPER_BOUND is defined.
//
// This escape hatch is only supported as a temporary means to give a bit more time
// for codebases to fix incorrect usages of associative containers.

// This precise test reproduces a comparator similar to boost::icl::exclusive_less
// for discrete right-open intervals: it orders disjoint intervals but treats
// overlapping intervals as equivalent, which is incorrect but was relied upon.

// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_LEGACY_TREE_LOWER_UPPER_BOUND
// UNSUPPORTED: c++03, c++11, c++14

#include <cassert>
Repository: llvm/llvm-project
File: clang/test/AST/ByteCode/codegen-constexpr-unknown.cpp (C++)

Code before:
// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s                                                  | FileCheck %s --check-prefix=CHECK
// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s -fexperimental-new-constant-interpreter -DINTERP | FileCheck %s --check-prefix=CHECK,INTERP

/// CodeGenFunction::ConstantFoldsToSimpleInteger() for the if condition
/// needs to succeed and return true.
extern void abort2();
constexpr const int* to_address(const int *p) {
  return p;
}
void rightscope() {
  const int p = 0;
  if (to_address(&p) == &p)
    return;
  abort2();
}
// CHECK:      define dso_local void @_Z10rightscopev()
// CHECK-NEXT: entry:
// CHECK-NEXT: %p = alloca i32
// CHECK-NEXT: store i32 0, ptr %p
// INTERP-NEXT: call noundef ptr @_Z10to_addressPKi(ptr noundef %p)


/// In the if expression below, the read from s.i should fail.
/// If it doesn't, and we actually read the value 0, the call to
/// func() will always occur, resuliting in a runtime failure.

struct S {
  mutable int i = 0;
};

void func() {
  __builtin_abort();
};

void setI(const S &s) {
  s.i = 12;
}

int main() {
  const S s;


Review comment (suggestion):
Can you add comments to this test, relates to the change, something like "p should be attached to the rightscope block scope and we can see this in the codegen by ...." 

Code after:
// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s                                                  | FileCheck %s --check-prefix=CHECK
// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -fcxx-exceptions -o - %s -fexperimental-new-constant-interpreter -DINTERP | FileCheck %s --check-prefix=CHECK,INTERP

/// CodeGenFunction::ConstantFoldsToSimpleInteger() for the if condition
/// needs to succeed and return true.
/// When re-visiting p during that evaluation, we need to attach the local
/// variable to the topmost scope, otherwise we will pick the call scope
/// of to_address and de-allocate the local variable at the end of the
/// to_address call.
/// FIXME: This is not currently correct since we still mark p as
/// constexpr-unknown and then reject it when comparing.
extern void abort2();
constexpr const int* to_address(const int *a) {
  return a;
}
void rightscope() {
  const int p = 0;
  if (to_address(&p) == &p)
    return;
  abort2();
}
// CHECK:      define dso_local void @_Z10rightscopev()
// CHECK-NEXT: entry:
// CHECK-NEXT: %p = alloca i32
// CHECK-NEXT: store i32 0, ptr %p
// INTERP-NEXT: call noundef ptr @_Z10to_addressPKi(ptr noundef %p)


/// In the if expression below, the read from s.i should fail.
/// If it doesn't, and we actually read the value 0, the call to
/// func() will always occur, resuliting in a runtime failure.

struct S {
  mutable int i = 0;
};

void func() {
  __builtin_abort();
};

void setI(const S &s) {
Repository: llvm/llvm-project
File: mlir/lib/Dialect/AMDGPU/Transforms/FoldMemRefsOps.cpp (C++)

Code before:
                         destIndices, memrefDest, "destination");

    if (failed(foldDstResult)) {
      memrefDest = op.getDst();
      destIndices = op.getDstIndices();
    }

    rewriter.replaceOpWithNewOp<GatherToLDSOp>(
        op, memrefSource, sourceIndices, memrefDest, destIndices,
        op.getTransferType(), op.getAsync());

    return success();
  }
};

struct FoldMemRefOpsIntoTransposeLoadOp final
    : OpRewritePattern<TransposeLoadOp> {
  using OpRewritePattern::OpRewritePattern;
  LogicalResult matchAndRewrite(TransposeLoadOp op,
                                PatternRewriter &rewriter) const override {
    Location loc = op.getLoc();

    SmallVector<Value> sourceIndices;
    Value memrefSource;

    if (failed(foldMemrefViewOp(rewriter, loc, op.getSrc(), op.getSrcIndices(),
                                sourceIndices, memrefSource, "source")))
      return failure();

    rewriter.replaceOpWithNewOp<TransposeLoadOp>(op, op.getResult().getType(),
                                                 memrefSource, sourceIndices);
    return success();
  }
};

void populateAmdgpuFoldMemRefOpsPatterns(RewritePatternSet &patterns,
                                         PatternBenefit benefit) {
  patterns
      .add<FoldMemRefOpsIntoGatherToLDSOp, FoldMemRefOpsIntoTransposeLoadOp>(
          patterns.getContext(), benefit);
}
} // namespace mlir::amdgpu


Review comment (bug):
nit: inline the location here, it's not used anywhere else
```suggestion
    if (failed(foldMemrefViewOp(rewriter, op.getLoc(), op.getSrc(), op.getSrcIndices(),
```

Code after:
                         destIndices, memrefDest, "destination");

    if (failed(foldDstResult)) {
      memrefDest = op.getDst();
      destIndices = op.getDstIndices();
    }

    rewriter.replaceOpWithNewOp<GatherToLDSOp>(
        op, memrefSource, sourceIndices, memrefDest, destIndices,
        op.getTransferType(), op.getAsync());

    return success();
  }
};

struct FoldMemRefOpsIntoTransposeLoadOp final
    : OpRewritePattern<TransposeLoadOp> {
  using Base::Base;
  LogicalResult matchAndRewrite(TransposeLoadOp op,
                                PatternRewriter &rewriter) const override {
    SmallVector<Value> sourceIndices;
    Value memrefSource;

    if (failed(foldMemrefViewOp(rewriter, op.getLoc(), op.getSrc(),
                                op.getSrcIndices(), sourceIndices, memrefSource,
                                "source")))
      return failure();

    rewriter.replaceOpWithNewOp<TransposeLoadOp>(op, op.getResult().getType(),
                                                 memrefSource, sourceIndices);
    return success();
  }
};

void populateAmdgpuFoldMemRefOpsPatterns(RewritePatternSet &patterns,
                                         PatternBenefit benefit) {
  patterns
      .add<FoldMemRefOpsIntoGatherToLDSOp, FoldMemRefOpsIntoTransposeLoadOp>(
          patterns.getContext(), benefit);
}
} // namespace mlir::amdgpu