[lldb-dev] clang::VersionTuple

Fri Jun 15 11:54:49 PDT 2018

> On Jun 15, 2018, at 3:44 AM, Pavel Labath via lldb-dev <lldb-dev at 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 <labath at 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 at lists.llvm.org> wrote:
>> 
>> 
>> 
>>> On May 8, 2018, at 11:37 AM, Greg Clayton <clayborg at 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 <clayborg at gmail.com> wrote:
>> 
>>> I'm good if Apple is good.
>> 
>>> On May 8, 2018, at 11:31 AM, Frédéric Riss <friss at apple.com> wrote:
>> 
>> 
>> 
>>> On May 8, 2018, at 10:04 AM, Greg Clayton via lldb-dev <
>> lldb-dev at lists.llvm.org> wrote:
>> 
>> 
>> 
>>> On May 8, 2018, at 9:47 AM, Zachary Turner <zturner at 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 at lists.llvm.org> wrote:
>> 
>>>> No issues from me.
>> 
>>>>> On May 8, 2018, at 9:11 AM, Pavel Labath via lldb-dev <
>> lldb-dev at 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 at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>> 
>>>> _______________________________________________
>>>> lldb-dev mailing list
>>>> lldb-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>> 
>> 
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>> 
>> 
>> 
>> 
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev