Details
- Reviewers
bader aaron.ballman
Diff Detail
Event Timeline
@zahiraam, could you rebase the patch to the tip of the branch, please? It looks like there are merge conflicts.
The patch LGTM, but I'd like someone else more familiar with the code to approve this change. I suggest finding someone who recently changes this code using git blame or adding code owners of these components.
I think we typically prefer not naming the parameter in the definition over a cast to void (and likely, we should be using [[maybe_unused]] in places where we currently cast to void because the variable is sometimes used, such as in an assert).
| llvm/include/llvm/ADT/DenseMapInfo.h | ||
|---|---|---|
| 0–1 | ||
| 0–1 | If it's complaining about lhs, I would imagine it would also be complaining about rhs? | |
| 1 | This looks suspicious -- rhs looks to be used below in the call to std:get<I>() and isEqualImpl<I + 1>(). Is this needed? | |
| llvm/include/llvm/ADT/Hashing.h | ||
| 0–1 | ||
| llvm/include/llvm/ADT/Optional.h | ||
| 0–1 | ||
| llvm/include/llvm/Support/MathExtras.h | ||
| 0–1 | ||
| llvm/include/llvm/ADT/DenseMapInfo.h | ||
|---|---|---|
| 1 | No that's no needed. | |
The changes look good as far as the diff is concerned, but the patch does not apply cleanly for me on ToT and the CI pipeline agrees (https://buildkite.com/llvm-project/diff-checks/builds/32992#831d7148-ce1c-4e47-9f96-e2d7edf67580).
@aaron Why is that? I did a rebase before applying my changes. How can I fix this? Should I rebase again and create another diff?
This is what I get with verbose output:
F:\llvm-project\llvm>git apply --verbose "F:\Aaron Ballman\Desktop\D98204.diff"
Checking patch llvm/include/llvm/ADT/DenseMapInfo.h...
error: while searching for:
}?
?
template <unsigned I>?
static unsigned getHashValueImpl(const Tuple &values, std::true_type) {?
return 0;?
}?
?
error: patch failed: llvm/include/llvm/ADT/DenseMapInfo.h:258
error: llvm/include/llvm/ADT/DenseMapInfo.h: patch does not apply
Checking patch llvm/include/llvm/ADT/Hashing.h...
error: while searching for:
?
template <typename... Ts, std::size_t... Indices>?
hash_code hash_value_tuple_helper(const std::tuple<Ts...> &arg,?
std::index_sequence<Indices...> indices) {?
return hash_combine(std::get<Indices>(arg)...);?
}?
?
error: patch failed: llvm/include/llvm/ADT/Hashing.h:656
error: llvm/include/llvm/ADT/Hashing.h: patch does not apply
Checking patch llvm/include/llvm/ADT/Optional.h...
error: while searching for:
return X != None;?
}?
?
template <typename T> constexpr bool operator<(const Optional<T> &X, NoneType) {
?
return false;?
}?
?
error: patch failed: llvm/include/llvm/ADT/Optional.h:381
error: llvm/include/llvm/ADT/Optional.h: patch does not apply
Checking patch llvm/include/llvm/Support/MathExtras.h...
error: while searching for:
return X < (UINT64_C(1) << (N));?
}?
template <unsigned N>?
constexpr inline std::enable_if_t<N >= 64, bool> isUInt(uint64_t X) {?
return true;?
}?
?
error: patch failed: llvm/include/llvm/Support/MathExtras.h:398
error: llvm/include/llvm/Support/MathExtras.h: patch does not applybut when I look at MathExtras.h:398, I see: return X < (UINT64_C(1) << (N)); (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/MathExtras.h#L398) So maybe whitespace? Ohhh, that seems to be it:
F:\llvm-project\llvm>git apply --ignore-whitespace "F:\Aaron Ballman\Desktop\D9204.diff"
F:\llvm-project\llvm>git status
On branch master
Your branch is up to date with 'origin/main'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: include/llvm/ADT/DenseMapInfo.h
modified: include/llvm/ADT/Hashing.h
modified: include/llvm/ADT/Optional.h
modified: include/llvm/Support/MathExtras.h
Untracked files:
(use "git add <file>..." to include in what will be committed)
out/
no changes added to commit (use "git add" and/or "git commit -a")