[Lldb-commits] [PATCH] D49018: Convert a location information from PDB to a DWARF expression

[Pavel Labath via lldb-commits]

I knew I should have stayed quiet :P, but now that I am in, here's my reasoning:

> - For eight years (the spaces were commited in 2010) some tests may have relied on these spaces. It's a good case - we can run tests and see it;
For seven of those eight years we haven't been using this function to
write tests. It was only used to produce debugging dumps to be viewed
by humans, and even that I'm not sure how often..

> - Some formatting may rely on these spaces and it will be broken. It's a worse case - if it wasn't covered with tests, then we will not see it;
What kind of "formatting"? If this is not some automated test (in
which case it would fall under your first item), then I don't think it
will care whether you have the extra space or not. Overall, I think
you're overestimating the importance of this function.

> - Format-only commits make the use of `git blame` more difficult. It's not very horrible, but still adds some inconvenients;
Yes, but without them you cannot make forward progress. (And the point
of my email here was to show that it's not just Stella and you(?) who
think removing the space looks better. I think it's an improvement
too).

Moreoever, I think that if there ever was a time to change the format
of the dump functions, it is now. A year ago we did not have any tests
based on this function. Now we have maybe two dozen.  Changing it now
is easier than next year when it may be hundreds. And I don't mean
just the spaces. If you see *anything* in the dump format that makes
writing a test, I encourage you to change it. I don't think we should
be afraid of change. LLVM shows is it's possible to change the textual
format even when you have tens of thousands of tests..


> - Several people in several places, not only in DWARFExpressions, use such a formatting style. May be it's an approved way of formatting such things?
> - The only thing I can find to justify such a commit is the beauty. But the beauty is the matter of opinion and what some peoples find beautiful, other find not.
Writing a space before a comma does not follow any stylistic rules I
know of and I would argue we should change all of those places.

On Thu, 19 Jul 2018 at 11:24, Aleksandr Urakov
<aleksandr.urakov at jetbrains.com> wrote:
>
> But can you, please, explain me why?
>
> Excuse me for so much fuss, I just wanted to explain my position... We have two cases now:
> - Just commit such a patch without an additional investigation. But it's not a good idea, because we can't be 100% sure that it is a typo (and even if it is, we still may break something from I listed above);
> - Make an additional investigation and commit this. But I think that it's unconstructive to do it now, because there are many problems with a higher priority. So I suggest to defer it for a better times. We may report a bug and assign it to me, I'll do it when I will finish other more important work.
>
> What do you think about it?
>
> On Thu, Jul 19, 2018 at 12:13 PM Pavel Labath <labath at google.com> wrote:
>>
>> On Thu, 19 Jul 2018 at 10:04, Aleksandr Urakov via Phabricator
>> <reviews at reviews.llvm.org> wrote:
>> > So peoples who commited such spaces found them beautiful (or used the approved formatting style, or used them for some special formatting cases),
>>
>> Or it's simply a typo.
>>
>> I don't want to make a bigger fuss of this than it already is, but I
>> think removing that space is a good idea.
>
>
>
> --
> Aleksandr Urakov
> Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop