Page MenuHomePhabricator

[NFC] Fix "unused parameter" error revealed in the Linux self-build.
ClosedPublic

Authored by zahiraam on Mar 8 2021, 11:36 AM.

Details

Diff Detail

Event Timeline

zahiraam created this revision.Mar 8 2021, 11:36 AM
zahiraam requested review of this revision.Mar 8 2021, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 11:36 AM
bader added a comment.Mar 10 2021, 2:47 AM

@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.

aaron.ballman added a subscriber: aaron.ballman.

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
zahiraam updated this revision to Diff 329721.Mar 10 2021, 11:41 AM
zahiraam marked 5 inline comments as done.Mar 10 2021, 11:41 AM
zahiraam added inline comments.
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).

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?

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 apply

but 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")
zahiraam updated this revision to Diff 329768.Mar 10 2021, 2:02 PM

Let's see if this time it worked. Thanks.

zahiraam updated this revision to Diff 330248.Mar 12 2021, 8:04 AM

@aaron.ballman Thanks for that. Let's see if this one will work.

aaron.ballman accepted this revision.Mar 12 2021, 10:18 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 12 2021, 10:18 AM