[llvm-dev] RFC: Modernizing our use of auto

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Tingyuan LIANG via llvm-dev


On Sun, Feb 3, 2019 at 6:50 AM Stephen Kelly via llvm-dev <[hidden email]> wrote:
On 31/12/2018 04:54, Chris Lattner via llvm-dev wrote:
>> Do those uses conform to the guide? If they don't, then should the guide be updated? Are the types there 'obvious’?
>
> If/when we revise the policy, then it would make sense for non-conforming uses of auto to be changed.  However, I don’t think that actually making a widespread change would be high priority...
>
>> How did all of those uses get into the codebase? Does it indicate that the guide is not followed, or does it indicate that the guide is too subjective, or that maybe the 'the type must be obvios' guide does not reflect the 'reality at the coalface' anymore? Should those uses of auto be changed?
>
> My understanding is that there has been no widely understood or accepted policy, so different coders and reviewers are doing different things.

One of the things which has no consensus here is whether 'auto' may be
used in lambdas (using c++-14).

Under the current guidelines, my understanding is that nothing prevents to use auto in lambda in order to "make the code more readable" when "the type is already obvious from the context" or "when the type would have been abstracted away anyways, often behind a container’s typedef such as std::vector<T>::iterator".

We don't need to update the guideline on auto to be able to use auto in lambda as soon as c++14 is available.

 
This feature was celebrated as a big
feature which gets unlocked by migrating to toolchains which provide
that feature:

  https://groups.google.com/forum/#!msg/llvm-dev/0VkIuhn10nE/QZ5FwYEmHAAJ

So, does this need a guideline update?

Is there consistency in celbrating that but writing 'remove all use of
auto from this file' in reviews?

If there's no consensus and no consistency, what does that mean for the
code?

Is

   if (const auto *TSI = D->getTypeSourceInfo())

ok?

Some reviewers say 'no'. What is the consensus and how is that expressed
in the guidelines?

Does anyone have any interest in making the guidelines more clear on
this?

I have made several proposals, and at least Chris agreed that something
should be improved, but then he left the discussion.

Does anyone else think that something can be improved? Is anyone willing
to read and comment on my proposal and get a change to the guidelines
committed?

I think that multiple people read your proposal and gave feedback on Phabricator that mandates a revision (for instance for-range loop). Also in this topic I believe some feedback was given that rewording in order to remove ambiguity is always good, as long as the "spirit" of the current rule as it is written is preserved.
So my take on the subject is that we're waiting on a new revision of your patch?

Best,

-- 
Mehdi


 

The original email in this thread was about how to handle features that
become 'unlocked' by updates to our minimum toolchain requirements. That
is now upon us.

Thanks,

Stephen.

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Tingyuan LIANG via llvm-dev


On 03/02/2019 17:59, Mehdi AMINI wrote:


On Sun, Feb 3, 2019 at 6:50 AM Stephen Kelly via llvm-dev <[hidden email]> wrote:
On 31/12/2018 04:54, Chris Lattner via llvm-dev wrote:
>> Do those uses conform to the guide? If they don't, then should the guide be updated? Are the types there 'obvious’?
>
> If/when we revise the policy, then it would make sense for non-conforming uses of auto to be changed.  However, I don’t think that actually making a widespread change would be high priority...
>
>> How did all of those uses get into the codebase? Does it indicate that the guide is not followed, or does it indicate that the guide is too subjective, or that maybe the 'the type must be obvios' guide does not reflect the 'reality at the coalface' anymore? Should those uses of auto be changed?
>
> My understanding is that there has been no widely understood or accepted policy, so different coders and reviewers are doing different things.

One of the things which has no consensus here is whether 'auto' may be
used in lambdas (using c++-14).

Under the current guidelines, my understanding is that nothing prevents to use auto in lambda in order to "make the code more readable" when "the type is already obvious from the context" or "when the type would have been abstracted away anyways, often behind a container’s typedef such as std::vector<T>::iterator".


Some people seem to have objections to use of auto with range-for loops too.

There doesn't seem to be consensus on what is 'readable'. Some people claim
strongly that 'the type must be obvious. There even seems to be consensus on
that phrase, though it doesn't seem to actually apply - We have lots of uses of
auto with AST Matchers for example, and the type of the matcher is not obvious
in that code. We have lots of similar 'the type is not "obvious"' code using auto.

And yet,

 if (const auto *TSI = D->getTypeSourceInfo())

draws review comments that it must be changed.


We don't need to update the guideline on auto to be able to use auto in lambda as soon as c++14 is available.


That seems to depend on the reviewer, which means the code and the reviews will be inconsistent.



 
This feature was celebrated as a big
feature which gets unlocked by migrating to toolchains which provide
that feature:

  https://groups.google.com/forum/#!msg/llvm-dev/0VkIuhn10nE/QZ5FwYEmHAAJ

So, does this need a guideline update?

Is there consistency in celbrating that but writing 'remove all use of
auto from this file' in reviews?

If there's no consensus and no consistency, what does that mean for the
code?

Is

   if (const auto *TSI = D->getTypeSourceInfo())

ok?

Some reviewers say 'no'. What is the consensus and how is that expressed
in the guidelines?

Does anyone have any interest in making the guidelines more clear on
this?

I have made several proposals, and at least Chris agreed that something
should be improved, but then he left the discussion.

Does anyone else think that something can be improved? Is anyone willing
to read and comment on my proposal and get a change to the guidelines
committed?

I think that multiple people read your proposal and gave feedback on Phabricator that mandates a revision (for instance for-range loop). Also in this topic I believe some feedback was given that rewording in order to remove ambiguity is always good, as long as the "spirit" of the current rule as it is written is preserved.
So my take on the subject is that we're waiting on a new revision of your patch?


We deliberately took the discussion off Phab and onto the mailing list
to try to get more-fruitful discussion. For example in


 http://clang-developers.42468.n3.nabble.com/Re-llvm-dev-RFC-Modernizing-our-use-of-auto-td4063365.html#a4063424


I suggested that 'New guidelines should...' and then wrote some
content. If we don't agree on 'what new guidelines should do', then
perhaps it is not time for a Phab patch yet?


Thanks for responding to that mail! :) I responded to you, but the
discussion again did not progress. Is there just not enough interest
in this? I'm getting the impression the topic should be dropped, but
maybe I'm missing something?


Thanks,

Stephen.



_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Tingyuan LIANG via llvm-dev
This might be a bit of a strange take, but as an outside user of LLVM, my personal request would be that uses of `auto` be loosely predicated on not breaking doxygen too badly/frequently. (I surmise this essentially means discouraging `auto` when there are member functions being called from it later to do interesting work, but I leave that for others to debate and decide).

I know doxygen already has many other similar failure modes (such as templates and auto-generated content, macros), but I thought offering this view might provide some objectivity to the "more readable" question, as it attempts to connect the definition of readability to a specific tooling question ("is the type available to a locally-syntactic tool with a particular purpose?") instead of subjective aesthetics (such as whether auto in lambda is more or less pretty).

Of course, that still leaves many edge cases: for example, I found
http://llvm.org/doxygen/IRBuilder_8h_source.html#l02247 while searching for an example for this email:
2247  if (const auto *CI = dyn_cast<ConstantInt>(OffsetValue))
2248  IsOffsetZero = CI->isZero();
2249

This is a case where auto is generally accepted (because the type is obvious in the dyn_cast), but since doxygen doesn't know about that convention, it is unable to find the documentation for the `isZero` call.

Anyways, my 2¢, from a lurker.


On Sun, Feb 3, 2019 at 1:31 PM Stephen Kelly via llvm-dev <[hidden email]> wrote:


On 03/02/2019 17:59, Mehdi AMINI wrote:


On Sun, Feb 3, 2019 at 6:50 AM Stephen Kelly via llvm-dev <[hidden email]> wrote:
On 31/12/2018 04:54, Chris Lattner via llvm-dev wrote:
>> Do those uses conform to the guide? If they don't, then should the guide be updated? Are the types there 'obvious’?
>
> If/when we revise the policy, then it would make sense for non-conforming uses of auto to be changed.  However, I don’t think that actually making a widespread change would be high priority...
>
>> How did all of those uses get into the codebase? Does it indicate that the guide is not followed, or does it indicate that the guide is too subjective, or that maybe the 'the type must be obvios' guide does not reflect the 'reality at the coalface' anymore? Should those uses of auto be changed?
>
> My understanding is that there has been no widely understood or accepted policy, so different coders and reviewers are doing different things.

One of the things which has no consensus here is whether 'auto' may be
used in lambdas (using c++-14).

Under the current guidelines, my understanding is that nothing prevents to use auto in lambda in order to "make the code more readable" when "the type is already obvious from the context" or "when the type would have been abstracted away anyways, often behind a container’s typedef such as std::vector<T>::iterator".


Some people seem to have objections to use of auto with range-for loops too.

There doesn't seem to be consensus on what is 'readable'. Some people claim
strongly that 'the type must be obvious. There even seems to be consensus on
that phrase, though it doesn't seem to actually apply - We have lots of uses of
auto with AST Matchers for example, and the type of the matcher is not obvious
in that code. We have lots of similar 'the type is not "obvious"' code using auto.

And yet,

 if (const auto *TSI = D->getTypeSourceInfo())

draws review comments that it must be changed.


We don't need to update the guideline on auto to be able to use auto in lambda as soon as c++14 is available.


That seems to depend on the reviewer, which means the code and the reviews will be inconsistent.



 
This feature was celebrated as a big
feature which gets unlocked by migrating to toolchains which provide
that feature:

  https://groups.google.com/forum/#!msg/llvm-dev/0VkIuhn10nE/QZ5FwYEmHAAJ

So, does this need a guideline update?

Is there consistency in celbrating that but writing 'remove all use of
auto from this file' in reviews?

If there's no consensus and no consistency, what does that mean for the
code?

Is

   if (const auto *TSI = D->getTypeSourceInfo())

ok?

Some reviewers say 'no'. What is the consensus and how is that expressed
in the guidelines?

Does anyone have any interest in making the guidelines more clear on
this?

I have made several proposals, and at least Chris agreed that something
should be improved, but then he left the discussion.

Does anyone else think that something can be improved? Is anyone willing
to read and comment on my proposal and get a change to the guidelines
committed?

I think that multiple people read your proposal and gave feedback on Phabricator that mandates a revision (for instance for-range loop). Also in this topic I believe some feedback was given that rewording in order to remove ambiguity is always good, as long as the "spirit" of the current rule as it is written is preserved.
So my take on the subject is that we're waiting on a new revision of your patch?


We deliberately took the discussion off Phab and onto the mailing list
to try to get more-fruitful discussion. For example in


 http://clang-developers.42468.n3.nabble.com/Re-llvm-dev-RFC-Modernizing-our-use-of-auto-td4063365.html#a4063424


I suggested that 'New guidelines should...' and then wrote some
content. If we don't agree on 'what new guidelines should do', then
perhaps it is not time for a Phab patch yet?


Thanks for responding to that mail! :) I responded to you, but the
discussion again did not progress. Is there just not enough interest
in this? I'm getting the impression the topic should be dropped, but
maybe I'm missing something?


Thanks,

Stephen.


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Tingyuan LIANG via llvm-dev
In reply to this post by Tingyuan LIANG via llvm-dev


On Sun, Feb 3, 2019 at 10:31 AM Stephen Kelly via cfe-dev <[hidden email]> wrote:


On 03/02/2019 17:59, Mehdi AMINI wrote:


On Sun, Feb 3, 2019 at 6:50 AM Stephen Kelly via llvm-dev <[hidden email]> wrote:
On 31/12/2018 04:54, Chris Lattner via llvm-dev wrote:
>> Do those uses conform to the guide? If they don't, then should the guide be updated? Are the types there 'obvious’?
>
> If/when we revise the policy, then it would make sense for non-conforming uses of auto to be changed.  However, I don’t think that actually making a widespread change would be high priority...
>
>> How did all of those uses get into the codebase? Does it indicate that the guide is not followed, or does it indicate that the guide is too subjective, or that maybe the 'the type must be obvios' guide does not reflect the 'reality at the coalface' anymore? Should those uses of auto be changed?
>
> My understanding is that there has been no widely understood or accepted policy, so different coders and reviewers are doing different things.

One of the things which has no consensus here is whether 'auto' may be
used in lambdas (using c++-14).

Under the current guidelines, my understanding is that nothing prevents to use auto in lambda in order to "make the code more readable" when "the type is already obvious from the context" or "when the type would have been abstracted away anyways, often behind a container’s typedef such as std::vector<T>::iterator".


Some people seem to have objections to use of auto with range-for loops too.

There doesn't seem to be consensus on what is 'readable'. Some people claim
strongly that 'the type must be obvious. There even seems to be consensus on
that phrase, though it doesn't seem to actually apply - We have lots of uses of
auto with AST Matchers for example, and the type of the matcher is not obvious
in that code. We have lots of similar 'the type is not "obvious"' code using auto.

And yet,

 if (const auto *TSI = D->getTypeSourceInfo())

draws review comments that it must be changed.


We don't need to update the guideline on auto to be able to use auto in lambda as soon as c++14 is available.


That seems to depend on the reviewer, which means the code and the reviews will be inconsistent.



 
This feature was celebrated as a big
feature which gets unlocked by migrating to toolchains which provide
that feature:

  https://groups.google.com/forum/#!msg/llvm-dev/0VkIuhn10nE/QZ5FwYEmHAAJ

So, does this need a guideline update?

Is there consistency in celbrating that but writing 'remove all use of
auto from this file' in reviews?

If there's no consensus and no consistency, what does that mean for the
code?

Is

   if (const auto *TSI = D->getTypeSourceInfo())

ok?

Some reviewers say 'no'. What is the consensus and how is that expressed
in the guidelines?

Does anyone have any interest in making the guidelines more clear on
this?

I have made several proposals, and at least Chris agreed that something
should be improved, but then he left the discussion.

Does anyone else think that something can be improved? Is anyone willing
to read and comment on my proposal and get a change to the guidelines
committed?

I think that multiple people read your proposal and gave feedback on Phabricator that mandates a revision (for instance for-range loop). Also in this topic I believe some feedback was given that rewording in order to remove ambiguity is always good, as long as the "spirit" of the current rule as it is written is preserved.
So my take on the subject is that we're waiting on a new revision of your patch?


We deliberately took the discussion off Phab and onto the mailing list
to try to get more-fruitful discussion. For example in


 http://clang-developers.42468.n3.nabble.com/Re-llvm-dev-RFC-Modernizing-our-use-of-auto-td4063365.html#a4063424


I suggested that 'New guidelines should...' and then wrote some
content. If we don't agree on 'what new guidelines should do', then
perhaps it is not time for a Phab patch yet?


Thanks for responding to that mail! :) I responded to you, but the
discussion again did not progress. Is there just not enough interest
in this?

That'd be my take on it - that some amount of readability varies depending on the reader (& will then vary depending on the reviewer, since they are the reader). Yeah, some subprojects of LLVM (LLD, LLDB are noteworthy here) have more esoteric/specialized styles than the rest of the LLVM project (but even within LLVM proper, different areas have some variations - maybe just due to history, not having been cleaned up for more modern (naming, style, C++ usage, etc) conventions).

Yes, it's a source of some friction when an existing LLVM developer (or a newcomer) moves into a different part of the project and writes what they believe are idiomatic - but finds a different local style requested - this is especially sub-optimal for newcomers where the friction is already fairly high to get involved.

But it's hard to muster enough buy-in to change much of that, to be honest.

- Dave
 
I'm getting the impression the topic should be dropped, but
maybe I'm missing something?


Thanks,

Stephen.


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
12