[Geany-devel] [CODE REVIEW] Changes to encodings

Colomban Wendling lists.ban at xxxxx
Wed May 18 13:28:27 UTC 2011


Le 18/05/2011 02:05, Matthew Brush a écrit :
> On 05/17/11 08:30, Colomban Wendling wrote:
>> Le 16/05/2011 05:45, Matthew Brush a écrit :
>>
>>> I was wondering if anyone had time to review some changes I've been
>>> working on?
>>
>> No really as you see, times is rare these days :/
>> But I'm trying to do my best...
>
> Thanks a lot for taking the time to review these changes, and also for
> using GitHub since it makes it *very* easy for me.  I will review them
> all and make the appropriate fixes after I finish checking my emails.

No problem, as far as I actually find the time to ^^

>> I did a fast and inaccurate review of the patch, see my comments on it:
>> https://github.com/codebrainz/geany/commit/81c89cce736c88f235761ce5765f2361fe2bedc8
>>
>>
>> yeah, I'm trying GitHub to see what it actually gives. But I'm afraid
>> it'll make my comments less visible for the ML readers, so unlikely fr
>> them to comment back…
> 
> I guess if more people were using it and watching each others
> repositories, everyone would be on the same page by getting
> notifications of such things.

Yeah probably, but that's not (yet?) the case ^^

>>> afterwards (though it's not such a big deal).  Also, I'm not completely
>>> satisfied with the name of the sci_add_text2() function added to the
>>> sciwrappers files, but there was already a function named sci_add_text()
>>> which truncates the text at the first NUL, and the new function is a
>>> more direct wrapper of the SCI_ADDTEXT message.
>>
>> sci_add_text_full() ?
>>
>> having counterpart for set_text() would probably be good, too.
> 
> I like the _full idea like GLib uses.  Regarding set_text(), it's
> actually one of the Scintilla functions that expects NUL termination.
> The reason for the add_text2() one is that add_text() was wrapped
> "wrong" (it's supposed to take a length param like in Scintilla) and I
> didn't want to clobber it for existing code.  That's not to say there
> couldn't be a set_text_full() which internally handles the case of
> embedded nulls by doing a clear_all() then a add_text() in the same way
> my changes do in document.c

That's what I meant for set_text_full(). Basically we don't need to show
the real SCI api here, just what's useful to us. If it means wrapping to
calls, I'd say it's just fine.
And having such a wrapper will help user (here meaning "client" code) to
properly give a length parameter where set_text() used to be used.

>>> These changes break the
>>> plugin API, and would require two small changes to the geany-plugins
>>> GeanyVC: geanyvc.c:480 and geanyvc.c:498 as well as GeanyGenDoc:
>>> ggd-plugin.c:343.  It should be obvious why this breakage was required I
>>> hope.
>>
>> yeah, sounds reasonable for such a change. However, prefer breaking the
>> API once (or at least it a single row), so I wouldn't see these
>> committed before further checking and other checks to whether other APIs
>> have to be updated.
> 
> Agreed, if I understand you correctly.

Hum, yeah, that must be the worst sentence I written since years :D
I just mean I don't want to break the ABI now, and again in two days (or
whatever short delay) because we figure out we have to change some other
API to support NULs in another place, and again and again.

Cheers,
Colomban



More information about the Devel mailing list