<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Nov 29, 2013 at 9:40 PM, Ben Longbons <span dir="ltr"><<a href="mailto:brlongbons@gmail.com" target="_blank" class="cremed">brlongbons@gmail.com</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Thu, Nov 28, 2013 at 1:21 AM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>> wrote:<br>



> Well, it is also quite rare that people declare multiple uninitialized<br>
> pointers in one line (and it would be a questionable practice).<br>
</div>It is extremely common. I checked both LLVM (1 thousand such<br>
declarations, sloccount 2 million) </blockquote><div><br></div><div>As Dmitri pointed out, there are quite a few false positives in there. Furthermore, I specifically mentioned uninitialized pointers. Almost all results of your query initialize the pointers (either by "=" or in a constructor initializer). You could do just the same with references and in fact there are several instances of multiple reference-type variables in one declaration.</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">and Linux (10 thousand / 10<br>
million) </blockquote><div><br></div><div>Isn't Linux a C codebase? That makes this is a non-issue for Linux and also explains the usage pattern (see below).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


using the following regex, which gives a few false positives<br>
but not many:<br>
[^/]\*.*, \*<br>
The proportion of about 1/1000 seems to be relatively constant, and<br>
caused by the relative lack of *need* for multiple pointers even among<br>
the few lines of code that are declarations, rather than an aberrant<br>
style.<br>
<div><br>
> My key argument against this style would be that it suggests that "*" and<br>
> "&" have a different binding behavior, which I don't think they have.<br>
</div>Sure, they don't have differences at the syntax level. But they *do*<br>
have significantly different usage patterns.<br></blockquote><div><br></div><div>I have doubts about this. And if they have this difference in usage patterns in your codebase, I'd personally consider that bad practice in C++. Leaving uninitialized pointers lying around is just bound to lead to bugs. Yes, back in the day (in C) when you needed to declare all variables at the beginning of a function, this might been a "good" and "benign" thing to do, but I don't think it is for C++.</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
> Well, it is not quite as easy:<br>
> 1. I am responsible for maintaining clang-format. Ensuring that all<br>
> combinations of options do the right thing gets significantly harder with<br>
> more options.<br>
</div>I don't think this is an issue with the patch, since all of the<br>
operations that it does now were done already, only the option checked<br>
changed.<br></blockquote><div><br></div><div>I estimate that less than 10% of the effort that this change will need are done at this point. Aside from needing decent unit testing (your patch has none), someone needs to attend to the bug reports that the "nastier cases" will create (and of course there are edge cases, e.g. do you format vector<int &> or vector<int&>?) and other formatting options get more difficult to implement correctly (e.g. someone else is proposing to support astyle's pointer style "middle").</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
> 2. Need to churn through pages and pages of flag documentation to<br>
> find the important flags makes it harder to use.<br>
</div>I expect that most people will either start with one of the predefined<br>
styles, or try to go through every option...<br>
<br>
However, it would be useful if the documentation for the two bits<br>
linked to each other.<br></blockquote><div><br></div><div>That is one thing. However, if you take a look at astyle's way of configuring this is that by default reference variables are attached the same way as pointers are (style equals "none"). I think this is a necessary choice as the vast majority of users will want the same behavior for both. So, by the end, we'll have two flags with several non-trivial values.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
That said - I found the documentation for clang-format rather lacking,<br>
since it just shows all the options in alphabetical order. It would be<br>
better if it were separated into logical areas (indent, mandatory<br>
lines, spaces, penalties and breaking lines) and *every* option had<br>
examples of each possible value.<br></blockquote><div><br></div><div>I totally agree and patches to the documentation are more than welcome. However, even with perfect documentation, fewer flags make usage easier.</div><div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div>
> 3. There are style choices that are bad or chosen for bad reasons. I think I<br>
> would put this one into this category. Now, if we support that style choice<br>
> in clang-format we ingrain it even further. If we don't (but clang-format<br>
> still delivers good value) we might convince a few people/projects to move a<br>
> way from this style.<br>
</div>Yes, my reasons were relatively weak and if I were to do it again, I<br>
would probably do it differently - if only to avoid trouble with<br>
formatting tools.<br></blockquote><div><br></div><div>So how about re-doing it now. clang-format can fix all instances for you quite easily. Yes, I know that there are additional costs (code churn, losing some of the blame history, ..). If clang-format provides enough of a carrot for some projects to abandon unfortunate style choices, I consider it a win.</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But it's not just an issue of forcing people to abandon their chosen<br>
style - it's an issue of whether they use a formatter at all.<br></blockquote><div><br></div><div>I am aware. However, if we get an additional 1% of users at the cost of making clang-format 1% less usable for the rest of the users, this is not an improvement. We don't have these numbers (I would think they are both far below 1%), so we can't really make an educated decision. </div>


<div><br></div><div>In addition, you are not really convinced of the style yourself. And I don't think we should carry forward every formatting choice that anyone has ever done.</div><div><br></div><div>So, my opinion remains that this is not something we want to support in clang-format. I am happy for others to weigh in, though.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div>
> Alp: Yes, I agree that "attach" or "align" (which astyle uses) would be<br>
> better names for this flag. But I am waiting to resolve a few questions<br>
> around this (e.g. there is also a request to support astyle's "middle"<br>
> alignment). We basically have to be backwards compatible, so I'd like to<br>
> change flags as seldomly as possible.<br>
</div>There is one backward-incompatibility with my patch as written: if the<br>
user explicitly specified PointerBindsToType instead of getting it<br>
from the BasedOnStyle.<br>
<br>
This would actually be a good opportunity to change the name of the<br>
option, with PointerBindsToType setting both PointerAttachToType and<br>
ReferenceAttachToType and possibly giving a deprecation warning.<br>
<br>
That said ... since there *will* be other options that need to be<br>
split in the future, you need a general plan, not a specific one.<br>
<br>
<br>
For reference, these are the options that astyle supports:<br>
pointers: unchanged (default), type, center, value<br>
references: like pointers (default), unchanged, type, center, value<br>
<br>
I haven't yet tested to see what exactly astyle does for some of the<br>
nastier cases.<br>
<br>
<br>
Implementing infrastructure like "do everything unchanged where<br>
possible" or </blockquote><div><br></div><div>This is fundamentally not how clang-format operates. It does this in few easy cases, but even those are proving to be difficult over and over again..</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
"generate the config file by --detect-style" is beyond<br>
me.<br></blockquote><div><br></div><div>Yeah, this is a whole separate project, if you ask me ..</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-o11c<br>
</blockquote></div><br></div></div>