llvm dev - Jul 2013 - [LLVMdev] [bikeshed] Anyone have strong feelings about always putting `template <...>` on its own line?

tl;dr If there are no objections I'd like to change clang-format's LLVM
style to always put `template <...>` on its own line. I think it's a
general code-layout consistency win and avoids some cases where trivial
code changes result in significant formatting differences (see the last
example).

Examples of the current behavior:

--------------
template <class ELFT>
class ELFState {

clang-format's to:

template <class ELFT> class ELFState {
--------------

--------------
template <class T>
static size_t vectorDataSize(const std::vector<T> &Vec) {

clang-format's to:

template <class T> static size_t vectorDataSize(const std::vector<T>
&Vec) {
--------------

Pathological example:
--------------
template <class T, size_t N, int X>
template <class U>
Foo<T, N, X>::bar(U Uv) {
  for (unsigned i = 0, e = Uv.size(); i != e; ++i)
    use(i);
}

clang-format's to:

template <class T, size_t N, int X> template <class U> Foo<T, N,
X>::bar(U
Uv) {
  for (unsigned i = 0, e = Uv.size(); i != e; ++i)
    use(i);
}

while the completely minor modification s/int/unsigned/ results (due to
line length restrictions) in clang-format agreeing with the former layout:

template <class T, size_t N, unsigned X>
template <class U>
Foo<T, N, X>::bar(U Uv) {
  for (unsigned i = 0, e = Uv.size(); i != e; ++i)
    use(i);
}
--------------

I would prefer that two pieces of code with similar logical structure to be
similarly formatted, as much as reasonable, and I think that always
breaking on template declarations is reasonable.

clang-format has an option for this (`AlwaysBreakTemplateDeclarations`),
but we don't have it enabled currently for LLVM style. Daniel Jasper
informs me that the current setting is a carry-over from before the setting
was introduced, where it was effectively always "false" (the current
setting for LLVM style).

I hate to bring up such a microscopic issue, but I find myself manually
fixing clang-format's behavior with LLVM style when it comes to putting
`template <...>` on its own line, since TBH I feel like a reviewer would
ask me to change it.

At a basic level I think what bothers me about it is that it breaks useful
invariants while reading code:
- A class or struct declaration always has `class` or `struct` as the first
non-whitespace character on the line (also helpful when grepping).
- When reading code, you can seek past the template by just going down to
the next line starting at the same indentation. If the `template <...>`
gets put on the same line, then you have to "parse through it" to find
what
is actually being declared.
- There is a single way to lay out `template <...>`. With the current
setting, clang-format will still put the `template <...>` on its own line
in a lot of cases due to line length restrictions, but in others it will be
put onto the same line. Hence in some cases you can "skip down past
it" and
in others you have to "parse through it", but worst of all you have to
detect which it is when reading the code.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130701/9794ef85/attachment.html>

2013/7/2 Sean Silva <silvas at purdue.edu>:
> always put `template <...>` on its own line.

+1

On Mon, Jul 1, 2013 at 2:21 PM, Sean Silva <silvas at purdue.edu>
wrote:
> tl;dr If there are no objections I'd like to change clang-format's
LLVM
> style to always put `template <...>` on its own line. I think
it's a general
> code-layout consistency win and avoids some cases where trivial code
changes
> result in significant formatting differences (see the last example).
>
> Examples of the current behavior:
>
> --------------
> template <class ELFT>
> class ELFState {
>
> clang-format's to:
>
> template <class ELFT> class ELFState {
> --------------
>
> --------------
> template <class T>
> static size_t vectorDataSize(const std::vector<T> &Vec) {
>
> clang-format's to:
>
> template <class T> static size_t vectorDataSize(const
std::vector<T> &Vec) {
> --------------
>
> Pathological example:
> --------------
> template <class T, size_t N, int X>
> template <class U>
> Foo<T, N, X>::bar(U Uv) {
>   for (unsigned i = 0, e = Uv.size(); i != e; ++i)
>     use(i);
> }
>
> clang-format's to:
>
> template <class T, size_t N, int X> template <class U>
Foo<T, N, X>::bar(U
> Uv) {
>   for (unsigned i = 0, e = Uv.size(); i != e; ++i)
>     use(i);
> }
>
> while the completely minor modification s/int/unsigned/ results (due to
line
> length restrictions) in clang-format agreeing with the former layout:
>
> template <class T, size_t N, unsigned X>
> template <class U>
> Foo<T, N, X>::bar(U Uv) {
>   for (unsigned i = 0, e = Uv.size(); i != e; ++i)
>     use(i);
> }
> --------------
>
> I would prefer that two pieces of code with similar logical structure to be
> similarly formatted, as much as reasonable, and I think that always
breaking
> on template declarations is reasonable.
>
> clang-format has an option for this (`AlwaysBreakTemplateDeclarations`),
but
> we don't have it enabled currently for LLVM style. Daniel Jasper
informs me
> that the current setting is a carry-over from before the setting was
> introduced, where it was effectively always "false" (the current
setting for
> LLVM style).
>
> I hate to bring up such a microscopic issue, but I find myself manually
> fixing clang-format's behavior with LLVM style when it comes to putting
> `template <...>` on its own line, since TBH I feel like a reviewer
would ask
> me to change it.
>
> At a basic level I think what bothers me about it is that it breaks useful
> invariants while reading code:
> - A class or struct declaration always has `class` or `struct` as the first
> non-whitespace character on the line (also helpful when grepping).
> - When reading code, you can seek past the template by just going down to
> the next line starting at the same indentation. If the `template
<...>` gets
> put on the same line, then you have to "parse through it" to find
what is
> actually being declared.
> - There is a single way to lay out `template <...>`. With the current
> setting, clang-format will still put the `template <...>` on its own
line in
> a lot of cases due to line length restrictions, but in others it will be
put
> onto the same line. Hence in some cases you can "skip down past
it" and in
> others you have to "parse through it", but worst of all you have
to detect
> which it is when reading the code.
Have you got any statistics for the current state of LLVM with respect
to this formatting issue? If something is already the overwhelmingly
common style (& it's not a case where it used to be the style, the
style has been updated, and nothing has been migrated yet) then just
make clang-format agree with reality - this doesn't require a
discussion or bikeshed.

On Mon, Jul 1, 2013 at 2:31 PM, David Blaikie <dblaikie at gmail.com>
wrote:
>
>
> Have you got any statistics for the current state of LLVM with respect
> to this formatting issue? If something is already the overwhelmingly
> common style (& it's not a case where it used to be the style, the
> style has been updated, and nothing has been migrated yet) then just
> make clang-format agree with reality - this doesn't require a
> discussion or bikeshed.
>
It's not overwhelming, but the preponderance seems to be towards putting it
on its own line (the exceptions are usually small trait specializations
like isPodLike). I give some rough numbers here <
http://thread.gmane.org/gmane.comp.compilers.llvm.devel/63378> (and see
Daniel's reply). Daniel is open to changing it, but asked me to gather some
more opinions.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130701/db1e764f/attachment.html>

On Mon, Jul 1, 2013 at 2:21 PM, Sean Silva <silvas at purdue.edu> wrote:
> tl;dr If there are no objections I'd like to change clang-format's
LLVM
> style to always put `template <...>` on its own line.
>
I would not like this change.

> I think it's a general code-layout consistency win and avoids some
cases
> where trivial code changes result in significant formatting differences
> (see the last example).
>
There are innumerable other such cases. I don't know that this in and of
itself is an important goal. Personally, I favor compactness and brevity of
reading the code slightly over this. I'm sure others will have different
opinions and valuations. I'm not sure there is a significant consensus
behind this particular issue.

I would prefer that two pieces of code with similar logical structure to
be
> similarly formatted, as much as reasonable, and I think that always
> breaking on template declarations is reasonable.
>
It's all in the trade offs you're willing to make. There are several
things
in clang-format today that will attempt a significantly different layout of
code in order to minimize the number of lines required even though it only
works when things happen to fit within 80 columns. I think those are still
worth having because often, things fit within 80 columns! =D

> I hate to bring up such a microscopic issue, but I find myself manually
> fixing clang-format's behavior with LLVM style when it comes to putting
> `template <...>` on its own line, since TBH I feel like a reviewer
would
> ask me to change it.
>
I, as a reviewer, would not ask you to change it. I can't recall i single
review where this has come up. I don't think this alone is a good
motivation.


I'll point out that while you give an extreme example in one direction,
there are extreme examples in the other direction as well:

template <int N> template <int M> struct my_trait_a : true_type {};
template <> template <> struct my_trait_a<1, 2> : false_type
{};
template <> template <> struct my_trait_a<2, 3> : false_type
{};
template <> template <> struct my_trait_a<3, 4> : false_type
{};
template <> template <> struct my_trait_a<4, 5> : false_type
{};

I don't really relish these constructs consuming 3x the number of lines.

Both of these are extreme cases, and they trade off differently. I think
for the common case neither solution is bad, and the current behavior
consumes slightly fewer lines of code. I think that's a reasonable stance
and would vote to keep it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130701/295df26e/attachment.html>