llvm dev - Oct 2017 - Adding a third-party dependency in clang-tools-extra

On Oct 19, 2017 6:50 PM, "Chandler Carruth" <chandlerc at
google.com> wrote:

On Thu, Oct 19, 2017 at 3:48 AM Sam McCall <sammccall at google.com>
wrote:
> clangd communicates with an editor via JSON-RPC. It parses JSON with
> YAMLParser, which is awkward, and generates JSON with printf and friends,
> which is miserable. Much of LLVM does things this way, but clangd does it a
> lot.
>
> I'd like to try replacing this with a JSON library. nlohmann/json[1]
seems
> like a reasonable fit: C++11 with exceptions optional, simple build, MIT
> license.
>
> I'd propose vendoring it under
tools/clang/tools/extra/clangd/nlohmann-json
> so there's no question of it "leaking" into runtimes as
described in this
> thread[2].
> This also means it wouldn't solve llvm's general JSON-parser
problem :-)
>
> Any LLVM-level objections or concerns? (Whether that library is the right
> technical choice for clangd can be discussed elsewhere, I think)
> If anyone wants to argue that we *shouldn't* bury it in
> clang/tools/extra/clangd, that's fine too!
>
Generally, I feel like we have continued to find JSON and YAML uses in
LLVM. I think it would be a shame to have more code in that space in the
repository.

But that brings us to another problem: outside of tests, we really
shouldn't add more third-party code with different licenses to LLVM. As a
compiler, LLVM has rather unique licensing requirements. So while this
might be "fine" inside of clangd, if it moves elsewhere (and in many
ways
it should!) it would become a problem. Worse, it might be easily missed, or
accidentally end up being used elsewhere.

Fair enough - we do need JSON facilities in many places.
I'm both surprised and unsurprised that licensing is a concern here :-)

I would personally be fairly reluctant to take this on unless there is a
pretty huge reason why it is needed. For example, if we had a absolute need
to do proper XML parsing and manipulation, the amount of code required for
that would be untenable without using one of the existing XML libraries.
LLDB for example actually does use an XML library IIRC.

I'm hoping that the problem domain here is substantially simpler and it is
tenable (if never really appealing) to just roll our own....
> Of course, my first inclination was to start writing one, and I had to
restrain myself!

Happy to have a crack at this and start a bikeshed thread over the design.
My main concerns:
 - compromising ease-of-use to satisfy every use case in LLVM. In
particular, I really want an eager parser rather than the streaming style
of YAMLparser.
 - a new library will need a test suite, benchmarks etc - sounds like using
third-party code for this is OK though.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171019/66642ad2/attachment.html>

On Thu, Oct 19, 2017 at 11:24 AM Sam McCall <sammccall at google.com>
wrote:
> On Oct 19, 2017 6:50 PM, "Chandler Carruth" <chandlerc at
google.com> wrote:
>
> On Thu, Oct 19, 2017 at 3:48 AM Sam McCall <sammccall at google.com>
wrote:
>
>> clangd communicates with an editor via JSON-RPC. It parses JSON with
>> YAMLParser, which is awkward, and generates JSON with printf and
friends,
>> which is miserable. Much of LLVM does things this way, but clangd does
it a
>> lot.
>>
>> I'd like to try replacing this with a JSON library.
nlohmann/json[1]
>> seems like a reasonable fit: C++11 with exceptions optional, simple
build,
>> MIT license.
>>
>> I'd propose vendoring it under
>> tools/clang/tools/extra/clangd/nlohmann-json so there's no question
of it
>> "leaking" into runtimes as described in this thread[2].
>> This also means it wouldn't solve llvm's general JSON-parser
problem :-)
>>
>> Any LLVM-level objections or concerns? (Whether that library is the
right
>> technical choice for clangd can be discussed elsewhere, I think)
>> If anyone wants to argue that we *shouldn't* bury it in
>> clang/tools/extra/clangd, that's fine too!
>>
>
> Generally, I feel like we have continued to find JSON and YAML uses in
> LLVM. I think it would be a shame to have more code in that space in the
> repository.
>
> But that brings us to another problem: outside of tests, we really
> shouldn't add more third-party code with different licenses to LLVM. As
a
> compiler, LLVM has rather unique licensing requirements. So while this
> might be "fine" inside of clangd, if it moves elsewhere (and in
many ways
> it should!) it would become a problem. Worse, it might be easily missed, or
> accidentally end up being used elsewhere.
>
> Fair enough - we do need JSON facilities in many places.
> I'm both surprised and unsurprised that licensing is a concern here :-)
>
> I would personally be fairly reluctant to take this on unless there is a
> pretty huge reason why it is needed. For example, if we had a absolute need
> to do proper XML parsing and manipulation, the amount of code required for
> that would be untenable without using one of the existing XML libraries.
> LLDB for example actually does use an XML library IIRC.
>
> I'm hoping that the problem domain here is substantially simpler and it
is
> tenable (if never really appealing) to just roll our own....
>
>> Of course, my first inclination was to start writing one, and I had to
> restrain myself!
>
> Happy to have a crack at this and start a bikeshed thread over the design.
> My main concerns:
>  - compromising ease-of-use to satisfy every use case in LLVM. In
> particular, I really want an eager parser rather than the streaming style
> of YAMLparser.
>
Did you look at the one that I referenced that was already in LLVM at some
point?

>  - a new library will need a test suite, benchmarks etc - sounds like
> using third-party code for this is OK though.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171019/22efc117/attachment.html>

On Thu, Oct 19, 2017 at 10:20 PM, Manuel Klimek <klimek at google.com>
wrote:
> On Thu, Oct 19, 2017 at 11:24 AM Sam McCall <sammccall at google.com>
wrote:
>
>> On Oct 19, 2017 6:50 PM, "Chandler Carruth" <chandlerc at
google.com> wrote:
>>
>> On Thu, Oct 19, 2017 at 3:48 AM Sam McCall <sammccall at
google.com> wrote:
>>
>>> clangd communicates with an editor via JSON-RPC. It parses JSON
with
>>> YAMLParser, which is awkward, and generates JSON with printf and
friends,
>>> which is miserable. Much of LLVM does things this way, but clangd
does it a
>>> lot.
>>>
>>> I'd like to try replacing this with a JSON library.
nlohmann/json[1]
>>> seems like a reasonable fit: C++11 with exceptions optional, simple
build,
>>> MIT license.
>>>
>>> I'd propose vendoring it under
tools/clang/tools/extra/clangd/nlohmann-json
>>> so there's no question of it "leaking" into runtimes
as described in this
>>> thread[2].
>>> This also means it wouldn't solve llvm's general
JSON-parser problem :-)
>>>
>>> Any LLVM-level objections or concerns? (Whether that library is the
>>> right technical choice for clangd can be discussed elsewhere, I
think)
>>> If anyone wants to argue that we *shouldn't* bury it in
>>> clang/tools/extra/clangd, that's fine too!
>>>
>>
>> Generally, I feel like we have continued to find JSON and YAML uses in
>> LLVM. I think it would be a shame to have more code in that space in
the
>> repository.
>>
>> But that brings us to another problem: outside of tests, we really
>> shouldn't add more third-party code with different licenses to
LLVM. As a
>> compiler, LLVM has rather unique licensing requirements. So while this
>> might be "fine" inside of clangd, if it moves elsewhere (and
in many ways
>> it should!) it would become a problem. Worse, it might be easily
missed, or
>> accidentally end up being used elsewhere.
>>
>> Fair enough - we do need JSON facilities in many places.
>> I'm both surprised and unsurprised that licensing is a concern here
:-)
>>
>> I would personally be fairly reluctant to take this on unless there is
a
>> pretty huge reason why it is needed. For example, if we had a absolute
need
>> to do proper XML parsing and manipulation, the amount of code required
for
>> that would be untenable without using one of the existing XML
libraries.
>> LLDB for example actually does use an XML library IIRC.
>>
>> I'm hoping that the problem domain here is substantially simpler
and it
>> is tenable (if never really appealing) to just roll our own....
>>
>>> Of course, my first inclination was to start writing one, and I had
to
>> restrain myself!
>>
>> Happy to have a crack at this and start a bikeshed thread over the
design.
>> My main concerns:
>>  - compromising ease-of-use to satisfy every use case in LLVM. In
>> particular, I really want an eager parser rather than the streaming
style
>> of YAMLparser.
>>
>
> Did you look at the one that I referenced that was already in LLVM at some
> point?
>
Yes - it seems a little easier to use than YAMLParser (forward iterators
rather than input iterators, easy to validate the whole document up front).
But the common things are still awkward, particular random access of object
properties.
And the ownership model (everything owned by the Parser) means you can't
use and compose JSON objects as value types.
(e.g. RequestContext::reply in https://reviews.llvm.org/D39098, where ID is
an arbitrary subtree of an earlier parsed document, and Result could/should
be passed by value).
Something closer to nlohmann seems worthwhile to me even if allocations
aren't optimal - but that might be a tough sell for a general LLVM support
lib.

On Thu, Oct 19, 2017 at 10:53 PM, Chandler Carruth <chandlerc at
google.com>
 wrote:
> On Thu, Oct 19, 2017 at 11:07 AM Manuel Klimek <klimek at google.com>
wrote:
>
>> If compelling, could an alternative be to make it a build time dep for
>> folks wanting to build clangd, as opposed to putting it in svn?
>>
>
> FWIW, while I don't like this personally, I also don't see any real
> problem with this.
>
Does this mean it somehow addresses the licensing issue? I don't really see
how, but this is hardly my area.
It does seem pretty inconvenient.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171019/855726c2/attachment.html>