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

[Shuxin Yang]

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.
That is different story.  On the other hand, it is not clear to as to if 
we really need to document it.
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!


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
>>>