<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<div class="moz-cite-prefix">On 31/12/2018 04:54, Chris Lattner
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<pre class="moz-quote-pre" wrap="">On Dec 16, 2018, at 11:44 AM, Stephen Kelly via llvm-dev <a class="moz-txt-link-rfc2396E" href="mailto:llvm-dev@lists.llvm.org"><llvm-dev@lists.llvm.org></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On 25/11/2018 14:43, Stephen Kelly via llvm-dev wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">However this is a proposal for more modern thinking regarding the permissiveness of auto in LLVM codebases.
Currently the rule on the use of auto is here:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Hi,
Thanks for the input on this topic, which has appeared here on the mailing list, on the phab review, and on IRC.
Almost all respondants generally expressed the claim "The type must be obvious if auto is used", though as I wrote before the guide uses auto in context that the type is not obvious:
for (const auto &Val : Container) { observe(Val); }
It seems that the respondants wish for 'almost never auto'. Fair enough, if the existing practice supports that.
There is one problem though, or rather ~56,000 problems. That's how many uses of auto there are in the entire codebase currently, according to someone on IRC.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I find this to be a helpful line of argument.</pre>
</blockquote>
<p><br>
</p>
<p>Given what you wrote below, maybe you are missing a negation
somewhere in this sentence?<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<pre class="moz-quote-pre" wrap="">We should, as a community, decide what the right thing is regardless of the existing art.</pre>
</blockquote>
<p><br>
</p>
<p>The existing art is part of 'the community deciding what to do'.</p>
<p><br>
</p>
<p>And yes, I think it makes sense to 'standardize existing
practice' where possible.<br>
</p>
<p><br>
</p>
<p>
</p>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
My understanding is that there has been no widely understood or accepted policy, so different coders and reviewers are doing different things.</pre>
</blockquote>
<p><br>
</p>
<p>And different contexts within LLVM are fine with auto, but seem
to get campaigning from other parts who have a different
interpretation of the guideline:</p>
<p><br>
</p>
<p><a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D33672#inline-475812">https://reviews.llvm.org/D33672#inline-475812</a></p>
<p><br>
</p>
<p>
</p>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">How is a new contributor to react to any of that? What are the real criteria that we can use to determine where auto will cause a patch to be rejected? Does it only depend on who you get as a reviewer?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Right now, it is quite arbitrary. This is a bug, not a feature.</pre>
</blockquote>
<p><br>
</p>
<p>Do we have some idea of who is interested in fixing the bug? It
can't be just one person fixing it - this is a community issue.
You've suggested that the guideline needs an update, and I've
already suggested an update. Is it only the two of us? How can we
proceed?</p>
<p><br>
</p>
<p>
</p>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Here is a quote from this thread from Chris and supported by Chandler and Quentin at least:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Perhaps the rule came be rewritten more generally, as
in “auto should generally only be used when the inferred
type is obvious from context, e.g. as a result of a cast
like dyn_cast or as a result of constructing a value with
a constructor containing a type name.”?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Is it reasonable to have that as a rule if there are ~12000 uses which break that rule?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<pre class="moz-quote-pre" wrap="">If you’d like to make progress on this, I think you should start by building consensus.</pre>
</blockquote>
<p><br>
</p>
<p>Well, I'm trying to find out what the positions people have are,
but even though there are so many existing usages of auto, this
thread is not getting responses from the people who put them
there. So, the code says one thing, and the guideline says
arguably the same thing, but people have alternative
interpretations of the guideline. But at least the responses here
are not really representative.<br>
</p>
<p>For me that means I'm not able to get my clang-query features
(<a class="moz-txt-link-freetext" href="https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/">https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/</a>)
upstream because I'm getting reviews which say "remove <tt
class="remarkup-monospaced">auto</tt>, here and everywhere in
this file." in</p>
<p><br>
</p>
<p> <a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D54408?id=173770#inline-480255">https://reviews.llvm.org/D54408?id=173770#inline-480255</a><br>
</p>
<p><br>
</p>
<p>That's a bit of a difficult review comment, given the ways it is
already used throughout the code.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20F584AE-6931-4321-8D32-5201F28E177F@nondot.org">
<pre class="moz-quote-pre" wrap="">It seems like there is widespread enough acknowledgement that the current state of things is broken, but there is no concrete proposal for a coding standards change. Please prepare a patch so we can discuss it.</pre>
</blockquote>
<p><br>
</p>
<p>I made a proposal in my initial mail in this thread. See the end
of the email:
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html">https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html</a></p>
<p>Phab is not well-suited to discussion like this, so we should
probably keep it on the mailing list for now.<br>
</p>
<p><br>
</p>
<p>But, here's some updated thinking:<br>
</p>
<p><br>
</p>
<p>New guidelines should <br>
<br>
* Be easy to follow<br>
* Have some consistency<br>
* Be modern<br>
* Be welcoming (or at least non-hostile) to newcomers<br>
* Standardize existing practice in LLVM<br>
* Achieve a consensus of support about the spirit of the guideline
(consensus is not unanimity) and be acceptable to people who
dislike auto<br>
<br>
</p>
<p>Can we agree on that much to start?<br>
</p>
<p><br>
</p>
<p>On the Phab review, some people expressed that they liked the
examples of when auto is acceptable. Here is an updated attempt at
guideline text for that section:<br>
</p>
<p><br>
<br>
```<br>
Some are advocating a policy of "almost always ``auto``" in C++11,
however LLVM<br>
uses a more moderate stance. Don't "almost always" use ``auto``,
but it may be used where either the Concept or the type is obvious
from the context. Here are<br>
some cases where use of ``auto`` would make sense:<br>
<br>
* Where the type appears elsewhere in the line (eg a dyn_cast)<br>
* Where the variable name or initializing expression provides
enough information (`auto SR = getSourceRange()`)<br>
* Where the context makes the *Concept* obvious, even if the type
is not obvious (eg, where the instance is only used as an
iterator, or in an algorithm as a container-like concept, or only
with a validity check, or an AST Matcher).<br>
<br>
Exceptions may arise, but they should only arise in exceptional
cases. If the case is not exceptional, apply the guidelines in
review discussion.<br>
```<br>
</p>
<p><br>
</p>
<p>The most important thing here is that it does not accept your
proposal that 'the type must be obvious'. Instead, it recognizes
that `auto` is really "an unspecified concept" - unspecified only
because we can't specify the concept in C++ yet.</p>
<p>However, the point/concern seems to be that readers of code
should know the instance may be used in its local scope.<br>
</p>
<p>That's why these guidelines would allow `auto Ctors` in <br>
</p>
<p><br>
</p>
<p>llvm::Optional<std::pair<std::string, MatcherCtor>><br>
getNodeConstructorType(ASTNodeKind targetType) {<br>
const auto &Ctors = RegistryData->nodeConstructors();<br>
auto It = llvm::find_if(<br>
Ctors, [targetType](const NodeConstructorMap::value_type
&Ctor) {<br>
return Ctor.first.isSame(targetType);<br>
});<br>
if (It == Ctors.end())<br>
return llvm::None;<br>
return It->second;<br>
} <br>
<br>
</p>
<p>because `Ctors` is obviously the Container *concept*, and knowing
exactly what type it is is not necessary in the local context.<br>
</p>
<p>However, in the below code it would probably not be ok because
we're calling methods on the instance which opens up more
possibilities (is it a base interface? etc):</p>
<p><br>
</p>
void SomeClass::foo(int input)<br>
{<br>
auto Ctors = getCtors(input);<br>
<br>
m_widgets = Ctors->calculate();<br>
}<br>
<br>
<p>Here, `Ctors` is definitely not a Container concept, we don't
know what kind of concept it is, so we should know the type by
seeing it typed in the code.<br>
</p>
<p>Another example from earlier in the thread:</p>
<p><br>
</p>
<p> template <typename BaseT, typename DerivedT><br>
void registerIfNodeMatcher(...) {<br>
auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();<br>
if (!NodeKind.isNone())<br>
NodeCtors[NodeKind] = std::make_pair(MatcherName,
Descriptor);<br>
}<br>
<br>
</p>
<p>Here, `NodeKind` is used as an Optional (or Maybe) *concept*. All
we do is a validity check. So, `auto` should be allowed here.</p>
<p>This 'the concept must be obvious' guideline is also what allows
the use of `auto` for iterators.<br>
</p>
<p><br>
</p>
<p>What do people think of "Either the Concept or the type should be
obvious from the context" as a baseline guideline?</p>
<p><br>
</p>
<p>Thanks,</p>
<p>Stephen.<br>
</p>
<p><br>
</p>
</body>
</html>