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

[Eric Christopher]

On Thu, Aug 22, 2013 at 12:37 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
> Hi, Eric:
>
>    Maybe I were not clear before.  What I tried to say was that I update the
> *original* comment to
> the tools/lto/LTOCodeGenerator.h to explicitly state all  boolean functions
> are ret-true-on-succ (see bellow).  Maybe we need to transform this comment
> into doxygen form.

Ah right. Yeah, wouldn't be a bad thing.

> That is different story.  On the other hand, it is not clear to as to if we
> really need to document it.

The default here is always "yes" ;)

> This class is a "local" class which dance behind lto_xxxx() APIs, and I
> did't touch the semantic
> of these APIs.
>
>   Hope this clarifies:-). Anyway thank you very much for paying attention to
> better code quality!
>

Thanks for the response. I just had this in my stack and wanted to
make sure it had been handled. Migrating those to doxygen would be
nice for sure.

Thanks.

-eric

>
> cat -n tools/lto/LTOCodeGenerator.h
>
>
>  82
>  83   // Write the merged module to the file specified by the given path.
>  84   // Return true on success.
>  85   bool writeMergedModules(const char *path, std::string &errMsg);
>  86
>
>  87   // Compile the merged module into a *single* object file; the path to
> object
>  88   // file is returned to the caller via argument "name". Return true on
>  89   // success.
>  90   //
>  91   // NOTE that it is up to the linker to remove the intermediate object
> file.
>  92   //  Do not try to remove the object file in LTOCodeGenerator's
> destructor
>  93   //  as we don't who (LTOCodeGenerator or the obj file) will last
> longer.
>  94   //
>  95   bool compile_to_file(const char **name, std::string &errMsg);
>  96
>
>
> Shuxin
>
>
> On 8/22/13 10:39 AM, Eric Christopher wrote:
>>
>> I didn't see if you ever added documentation for the functions you
>> changed. Did you?
>>
>> -eric
>>
>> On Mon, Aug 12, 2013 at 5:41 PM, Eric Christopher <echristo at gmail.com>
>> wrote:
>>>
>>> On Mon, Aug 12, 2013 at 5:32 PM, Shuxin Yang <shuxin.llvm at gmail.com>
>>> wrote:
>>>>
>>>> I guess I answer your question in the mail I replied to David:-)
>>>>
>>>> As of today,  the LTOCodeGenerator is consistently
>>>> "return-true-on-succ".  I
>>>> updated the comments to reflect the change.
>>>> (The comment was actually added by me).  This class has no document at
>>>> all,
>>>> only comments.  Part of the reason I guess
>>>>   is that it is "internal" class. It is called by lto_xxx APIs.  I
>>>> didn't
>>>> change the semantics of these APIs. I definitely cannot
>>>> change them -- otherwise, we are not able to compile with Apple-ld.
>>>>
>>>> I believe this class has no ambiguity. Of course it is my biased
>>>> opinion. If
>>>> you have specific advice as to how to improve the
>>>> interface. I'm open to make change. It should not be a big deal I guess.
>>>>
>>> Aww. Missed that. Guess the functions could all use some documentation
>>> in a block comment? :)
>>>
>>> -eric
>>>
>>>> On 8/12/13 5:04 PM, Eric Christopher wrote:
>>>>>
>>>>> Ping?
>>>>>
>>>>> -eric
>>>>>
>>>>> On Thu, Aug 8, 2013 at 1:07 PM, Eric Christopher <echristo at gmail.com>
>>>>> wrote:
>>>>>>>
>>>>>>> As to consistency. Yes, consistency is important. That is why I need
>>>>>>> to
>>>>>>> change
>>>>>>> the interface.  On the other hand, maybe I were not clear before.
>>>>>>> What I
>>>>>>> commit
>>>>>>> in this revision only change the private functions. I'm working on
>>>>>>> the
>>>>>>> two
>>>>>>> public functions
>>>>>>> as well.
>>>>>>>
>>>>>>> As to documentation, in yesterday's commit. I clearly high-light the
>>>>>>> return-on-false
>>>>>>>
>>>>>> My complaint here is that your patch changed the return of numerous
>>>>>> functions without seemingly needing a documentation change. My request
>>>>>> was that either a) you should have changed the documentation, or b) if
>>>>>> it doesn't exist add some documentation to all of the functions you
>>>>>> just changed stating what the expected return is.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> -eric
>>>>
>>>>
>