> On Jun 15, 2018, at 3:44 AM, Pavel Labath via lldb-dev
> <lldb-dev@lists.llvm.org> wrote:
>
> Hello again,
>
> Just a quick update on the state of this.
>
> I've managed to move VersionTuple from clang to llvm. I've also
> created <https://reviews.llvm.org/D47889> to switch over our version
> handling to that class.
>
> Could I interest anyone in taking a quick look at the patch?
Somehow I can’t log into Phabricator from home so I can’t comment right now,
but I took a look.
In some of your changes in the SB files you do:
if (PlatformSP platform_sp = GetSP())
version = platform_sp->GetOSVersion();
I don’t like putting initializers in if statements like this because I always
have to think twice about whether you meant “==“. Moreover, all of the
surrounding code does it differently:
PlatformSP platform_sp = GetSP()
if (platform_sp)
version = platform_sp->GetOSVersion();
so switching to the other form in a couple of places only kinda forces the
double-take. But that’s a little nit.
Given that clang::VersionTuple::tryParse does what the hand-parsing in place
(which I’m assuming it does or you wouldn’t have used it) this looks fine and a
nice cleanup to me.
Jim
>
> pl
>
> On Wed, 9 May 2018 at 08:49, Pavel Labath <lab...@google.com> wrote:
>>
>> Thank you all for the feedback. I'll get on with this when I find some
>> spare time. I will send a patch which will show the final look of the code,
>> before I start moving VersionTuple into llvm.
>>
>> cheers,
>> pl
>> On Tue, 8 May 2018 at 19:46, Frédéric Riss via lldb-dev <
>> lldb-dev@lists.llvm.org> wrote:
>>
>>
>>
>>> On May 8, 2018, at 11:37 AM, Greg Clayton <clayb...@gmail.com> wrote:
>>
>>> I was referring to the Swift and Apple internal branches. They tend to
>> lock down against older llvm and clang repositories so when we put changes
>> in llvm or clang that are required for LLDB, it makes merging a bit tougher
>> in those cases. Again, I am not affected by this, just trying to watch out
>> for you guys.
>>
>>
>>> I understand and I appreciate it, I was just worried that I’m missing
>> something obvious. We branch LLDB at the same time as LLVM so that’s not
>> actually an issue. Of course, it might cause merge conflicts or make it
>> harder to cherry-pick patches, but that's just living downstream.
>>
>>> Fred
>>
>>> Greg
>>
>>
>>> On May 8, 2018, at 11:35 AM, Greg Clayton <clayb...@gmail.com> wrote:
>>
>>> I'm good if Apple is good.
>>
>>> On May 8, 2018, at 11:31 AM, Frédéric Riss <fr...@apple.com> wrote:
>>
>>
>>
>>> On May 8, 2018, at 10:04 AM, Greg Clayton via lldb-dev <
>> lldb-dev@lists.llvm.org> wrote:
>>
>>
>>
>>> On May 8, 2018, at 9:47 AM, Zachary Turner <ztur...@google.com> wrote:
>>
>>> We don’t want the lowest levels of lldb to depend on clang. If this is
>> useful we should move it from clang to llvm and use llvm::VersionTuple
>>
>>
>>> I agree, though this move will cause merging issues for many that have
>> repositories that link against older llvm/clang. Doesn't affect me anymore,
>> but Apple will be affected.
>>
>>
>>> I’m not sure I understand what issues you’re referring to, we don’t link
>> new LLDBs to old clangs (and even if we did, it wouldn’t be something the
>> that drives community decisions).
>>
>>> Fred
>>
>>> Greg
>>
>>> On Tue, May 8, 2018 at 9:26 AM Greg Clayton via lldb-dev <
>> lldb-dev@lists.llvm.org> wrote:
>>
>>>> No issues from me.
>>
>>>>> On May 8, 2018, at 9:11 AM, Pavel Labath via lldb-dev <
>> lldb-dev@lists.llvm.org> wrote:
>>>>>
>>>>> While moving Args around, I noticed that we have a bunch of
>>>>> functions/classes that pass/store version numbers as a triplet of
>> integers
>>>>> (e.g. Platform::GetOSVersion). I got halfway into creating a wrapper
>> class
>>>>> for that when I noticed clang::VersionTuple, which is pretty much what
>> I
>>>>> wanted out of the box.
>>>>>
>>>>> Now there are small differences between this class, and what we have
>> now:
>>>>> it has an extra fourth "build" field, and it uses only 31 bits to
>> represent
>>>>> the values. None of these seem to matter (particularly as we are
>>>>> converting our representation into this struct in some places) that
>> much,
>>>>> but before I go through the trouble of pulling this class into llvm
>>>>> (although technically possible, it seems wrong to pull a clang
>> dependency
>>>>> at such a low level), I wanted to make sure we are able to use it.
>>>>>
>>>>> Do you see any reason why we could not replace our version triplets
>> with
>>>>> clang::VersionTuple ?
>>>>>
>>>>> cheers,
>>>>> pl
>>>>> _______________________________________________
>>>>> lldb-dev mailing list
>>>>> lldb-dev@lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>>>> _______________________________________________
>>>> lldb-dev mailing list
>>>> lldb-dev@lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>>
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>>
>>
>>
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> _______________________________________________
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
