std::tie for local variables that are non-modifying is no longer needed in
C++17. Replace these instances with structured bindings.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp | ||
---|---|---|
426–429 | We could also do this, right? Not sure how that formats though. |
I have some readability concerns, I think this should be solved by an RFC on the style guide.
mlir/examples/toy/Ch7/mlir/MLIRGen.cpp | ||
---|---|---|
459 ↗ | (On Diff #451711) | I see this kind of change as a regression in terms of readability: the types are no longer explicit and we're forced into auto instead. |
Seems like the standard hasn't moved on https://isocpp.org/files/papers/p0480r1.html :(
I think going from
if (auto it : llvm::zip(a, b)) thing(std::get<0>(it), std::get<1>(it));
to
if (auto [x, y] : llvm::zip(a, b)) thing(x, y);
This at least is unambiguously better I feel.
Right, this is a different case as there was already auto. This patch is about replacing std::tie as far as I can tell.
Yes, this patch is targeting the few uses of std::tie in contexts that don't modify references to function local or class member variables. Most of the std::tie uses, unfortunately, fall into the category I just mentioned around references.
When poking around, I also had the same observation of the use of explicit std::get<N> in lieu of structured bindings. I agree with you that structured binding use is clearly better in those cases. I intentionally didn't couple those patterns with this patch; I'm happy to explore those in a separate patch as it improves readability IMO. How does that sound?
For example (no pun intended), in the examples, we'd replace
for (const auto nameValue : llvm::zip(protoArgs, entryBlock.getArguments())) { if (failed(declare(std::get<0>(nameValue)->getName(), std::get<1>(nameValue)))) return nullptr; }
with
for (const auto [name, value] : llvm::zip(protoArgs, entryBlock.getArguments())) { if (failed(declare(name->getName(), value))) return nullptr; }
mlir/examples/toy/Ch7/mlir/MLIRGen.cpp | ||
---|---|---|
459 ↗ | (On Diff #451711) | I'll revert this part of the patch. I agree this isn't clearly better and we should strive for readability and understanding of types with these examples. |
mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp | ||
426–429 | In general, yes, that's a safe change. I don't think we should necessarily do that here though. Later on in this function, we do Value originalBound = std::get<3>(config), so we'd need to copy the upperBound value for use later on if we remove the config variable, but the local variable definition would have to be a bit more removed from its use which is not ideal (we'd need to modify it before potentially changing the upperBound in one of the branches). I think I'll keep the code as-is unless you feel strongly. |
I'm less concern about the StringRef case (I guess it is obvious enough that "splitting" a StringRef returns two StringRef).
mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp | ||
---|---|---|
431 | We're losing Type qualifier here as well... |
Given the concerns about losing the explicit type information for variable declarations, I'm no longer going to pursue these changes.
I'll look into the zip/std::get<N> use in a different patch and we can see what we think of those use of structured bindings. Most of those from a quick grep around already use auto so it won't be a regression in terms of type information to readers.
We could also do this, right? Not sure how that formats though.