This is an archive of the discontinued LLVM Phabricator instance.

Using auto for calls to cast-like functions to cover LLVM style guide.
AcceptedPublic

Authored by staronj on Dec 10 2016, 5:21 AM.

Details

Summary

Changes in LLVM code to satisfy Use auto Type Deduction to Make Code More Readable rule in LLVM coding standards.

Diff is produced by prototype check that is similar to enchantments made by malcolm.parsons in D27166 (modernize-use-auto).

Diff Detail

Event Timeline

staronj updated this revision to Diff 80998.Dec 10 2016, 5:21 AM
staronj retitled this revision from to Changes in LLVM after running llvm-use-auto-in-calls check..
staronj updated this object.
staronj added a reviewer: Prazek.

It's reasonable application of Clang-tidy, but I think will be better to split fixes in several commits based on functionality.

Did you also enable analysis of headers?

Could you please clarify situation with check? Similar thing is implemented in D27166.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: malcolm.parsons.

Forgive me if this question is too general or has an obvious answer...

I am wondering what the advantage is of using the auto type specifier in place of actual type specifiers in general. I certainly understand it when it comes to overly verbose iterator declarations and nested types, but I fail to see the advantage of applying such a change as widely as this.

So I am not fundamentally opposed to this change, but I would like to understand the rationale a bit more clearly rather than seeing a patch that has no description other than "results of running clang-tidy on LLVM source". As a PowerPC back end developer I spend a lot of time in that code and as I look at this patch, I do not see it as an obvious improvement to the code that is currently there. That is not to say I see any changes that hinder readability since it appears to only be applied to calls where the the explicit template argument specifies the return type.

Forgive me if this question is too general or has an obvious answer...

I am wondering what the advantage is of using the auto type specifier in place of actual type specifiers in general.

In general: none.

I certainly understand it when it comes to overly verbose iterator declarations and nested types, but I fail to see the advantage of applying such a change as widely as this.

So I am not fundamentally opposed to this change, but I would like to understand the rationale a bit more clearly rather than seeing a patch that has no description other than "results of running clang-tidy on LLVM source".

I agree this a poorly described patch.

As a PowerPC back end developer I spend a lot of time in that code and as I look at this patch, I do not see it as an obvious improvement to the code that is currently there. That is not to say I see any changes that hinder readability since it appears to only be applied to calls where the the explicit template argument specifies the return type.

From I saw in this patch, it is implementing this: http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable : do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context

Prazek edited edge metadata.Dec 12 2016, 3:01 AM

Could you please clarify situation with check? Similar thing is implemented in D27166.

We had race condition in fixing this bug and Malcolm published his solution before us. The check is doing the same thing.
Jakub, maybe you should remove check name from commit, because it won't gonna land in trunk with this name.

It's reasonable application of Clang-tidy, but I think will be better to split fixes in several commits based on functionality.

Did you also enable analysis of headers?

What is the good way to split it in your opinion? These are changes to only this one case (auto * I = dyn_cast<Instr> etc), so there is only one functionality.

Forgive me if this question is too general or has an obvious answer...

I am wondering what the advantage is of using the auto type specifier in place of actual type specifiers in general.

In general: none.

I certainly understand it when it comes to overly verbose iterator declarations and nested types, but I fail to see the advantage of applying such a change as widely as this.

So I am not fundamentally opposed to this change, but I would like to understand the rationale a bit more clearly rather than seeing a patch that has no description other than "results of running clang-tidy on LLVM source".

I agree this a poorly described patch.

As a PowerPC back end developer I spend a lot of time in that code and as I look at this patch, I do not see it as an obvious improvement to the code that is currently there. That is not to say I see any changes that hinder readability since it appears to only be applied to calls where the the explicit template argument specifies the return type.

From I saw in this patch, it is implementing this: http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable : do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context

Exactly. Having redundant type is 1) redundant 2) bugprone (someone could change single type when modyfying source code).
I saw that in most of the cases reviewers are enforcing that rule. The diff is so large because LLVM was started before c++11

What is the good way to split it in your opinion? These are changes to only this one case (auto * I = dyn_cast<Instr> etc), so there is only one functionality.

Yes, because it is largely automatic and the pattern is well identified, I'm fine with a single patch. But please update the description!

staronj retitled this revision from Changes in LLVM after running llvm-use-auto-in-calls check. to Using auto for calls to cast-like functions to cover LLVM style guide..Dec 13 2016, 6:43 AM
staronj updated this object.
staronj edited edge metadata.
mehdi_amini accepted this revision.Dec 13 2016, 8:32 AM
mehdi_amini added a reviewer: mehdi_amini.

LGTM.

This revision is now accepted and ready to land.Dec 13 2016, 8:32 AM