[llvm] r187819 - Change private functions of LTOCodeGenerator from ret-false-on-succ to ret-true-on-succ.

David Blaikie dblaikie at gmail.com
Tue Aug 6 16:10:42 PDT 2013


On Tue, Aug 6, 2013 at 3:55 PM, Eric Christopher <echristo at gmail.com> wrote:
> On Tue, Aug 6, 2013 at 3:51 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
>>
>> On 8/6/13 3:40 PM, Eric Christopher wrote:
>>>
>>> On Tue, Aug 6, 2013 at 3:37 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
>>>>
>>>>
>>>>> To whom?
>>>>
>>>> To everybody who feel more natural with return-true-on-succ.
>>>
>>> Then why aren't you proposing a standard for the project and changing
>>> everything?
>>
>> As far as I can tell from the community's feedback. Most people take for
>> granted
>> return-true-on-succ is more natural. Now that it is widely accepted, why
>> mess around
>> proposing such std?
>>
>
> Because it's more clear that way as to what should happen.

To expound on this a little further (in agreement with Eric): There's
no current predominant style & both true and false successes are used
throughout the codebase. Switching them back & forth isn't a great
idea in the absence of a style notation so that newcomers to the
codebase know which style to follow, which ones to leave alone, and
which ones to cleanup.

The style guide also says to be consistent (pretty sure) - so without
documentation either way, I'd say the style guide says to do what the
existing code does, rather than to change it. It's not hard to add
some wording in there to codify what you see as universal agreement
(I'm not sure it's that, but at least people can discuss that in a
style guide change).

>
>> One thing not clear is that how to deal with existing return-false-on-succ
>> code.
>> My take (as embodied by this change) is to modify the code whenever and
>> wherever
>> one feel comfortable.  Seems like you are not happy with such approach.  How
>> about
>> you come up some guidelines and codify it, such that we can follow without
>> ambiguity.
>>
>
> Because I'm not the one who brought it up in the first place - and as
> you obviously don't remember I agreed with you that it was confusing.
>
>>
>>>>
>>>>> The code that you'd be changing hasn't been committed yet.
>>>>>
>>>> I don't think it is better to commit confusing integrated code first,
>>>> then
>>>> get rid of confusing part.
>>>
>>> This isn't what I'm saying at all. I'm saying that you should change
>>> the code that you're writing to match the code that you are
>>> integrating with since your code hasn't been approved or committed
>>> yet. Then, in the future, if we want to change the meaning of the APIs
>>> all of it can change.
>>
>> As I reiterate in my previous mail, I don't won't to change the API -- Apple
>> linker
>> directly call these APIs, there is no way to change it; on the other hand,
>> changing API is dangerous.
>
> I know exactly what ld64 does here and changing the C API is
> unacceptable anyhow unless we version and deprecate. Which probably
> makes more sense to do if you have a bunch of changes and extensions
> coming.
>
> At any rate the review feedback that I do want you to follow is:
>
> Write documentation for each of these functions that specifies what
> the return value is and what it means in each case.
> If you care enough to change it then you care enough to document it.

Yep - regardless of whether the style guide is modified to support
this or not, we have both styles in the codebase today (& will do for
a while yet, even if we make a concerted effort to move towards one
style here) & need to document which way any API goes.



More information about the llvm-commits mailing list