This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Reverting invalid D135126
AbandonedPublic

Authored by wrengr on Oct 4 2022, 11:47 AM.

Details

Summary

The change D135126 should never have landed, since the warnings in
question were already resolved by D134925.

Diff Detail

Event Timeline

wrengr created this revision.Oct 4 2022, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 11:47 AM
wrengr requested review of this revision.Oct 4 2022, 11:47 AM
aartbik added inline comments.Oct 4 2022, 12:04 PM
mlir/lib/ExecutionEngine/SparseTensor/File.cpp
27

please revert BAZEL fix as part of this change too

D135126 was needed to fix the Windows mlir bot which was red for several days because of https://github.com/llvm/llvm-project/commit/e898be2f6edbb886af2f6b23e2f5db5210535620 which landed after D134925 and undid it.

NathanielMcVicar added inline comments.
mlir/lib/ExecutionEngine/SparseTensor/File.cpp
111

I think it's probably irrelevant after D135304, but putting this in the default produces a clang warning (see discussion in D135126).

Bah, when D135126 came to my attention I thought it had already landed (not just been approved). But since it hasn't, there's no need for this differential— supposing we can get D135126 into a state that all three of MSVC, Clang, and Bazel all like it before it does land

D135126 was needed to fix the Windows mlir bot which was red for several days because of https://github.com/llvm/llvm-project/commit/e898be2f6edbb886af2f6b23e2f5db5210535620 which landed after D134925 and undid it.

I missed that D134925 got reverted. Thanks for the heads-up

wrengr abandoned this revision.Oct 6 2022, 2:17 PM

This differential is no longer necessary. The changes in D135304 got everything into a good state for MSVC, Clang, and our team's style goals. And D135401 will revert the Bazel dependencies.