[lld] r181492 - [lld] Add comments to InputFiles::searchLibraries() arguments.

[Rui Ueyama]

On Wed, May 8, 2013 at 6:57 PM, Nick Kledzik <kledzik at apple.com> wrote:

> I'm happy with a separate line for each parameter with a trailing comment
> that Rui added.
>
> But, another way to make this more readable is to create a SearchOptions
> struct and use it to hold the bool parameters.  So the client code becomes:
>
> InputFiles:SearchOptions options.
>  options.searchSharedLibs = true;
> options.searchArchives = true;
> options.dataSymbolOnly = false;
>  _inputFiles.searchLibraries(undefinedName, options, *this);
>

Ah, I like that. I'm not sure if making another change worth doing, but
that's more readable.

Rui

-Nick
>
>
> On May 8, 2013, at 6:28 PM, Rui Ueyama <ruiu at google.com> wrote:
>
> I think LLD does not have another convention for comments on arguments.
> Added Michael to confirm.
>
> I originally tried to make it a bitmask, but I didn't like the resulting
> code, because I needed to make a bitmask with code like this in this
> function:
>
>  (searchArchives ? SLK_Archives : 0) | (searchSharedLibs ? SLK_SharedLibs
> : 0) | SLK_dataSymbolOnly
>
> The repeated (x ? y : 0) pattern looks too verbose too me, so I thought
> just adding comments would be better.
>
>
> On Wed, May 8, 2013 at 6:02 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
>> On Thu, May 9, 2013 at 1:54 AM, Rui Ueyama <ruiu at google.com> wrote:
>>
>>> Author: ruiu
>>> Date: Wed May  8 18:54:10 2013
>>> New Revision: 181492
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=181492&view=rev
>>> Log:
>>> [lld] Add comments to InputFiles::searchLibraries() arguments.
>>>
>>
>> I'm not sure this is really the best approach, and if it is it would be
>> better to follow the conventions for comments on arguments in the rest of
>> LLVM (unless LLD has *another* divergent convention here??).
>>
>> Specifically, could we instead use the (very common in LLVM and Clang)
>> pattern of:
>>
>> foo(/*Arg1=*/ true, /*Arg2=*/ false, ...);
>>
>>
>> Maybe even better would be to provide interfaces that don't require so
>> many booleans. For example, here it seems like a bitmask enum would be
>> nicely self-documenting?
>>
>> _inputFiles.searchLibraries(undefName, SLK_SharedLibs | SLK_Archives |
>> ...);
>>
>> Just something to consider.
>>
>>
>>>
>>> Modified:
>>>     lld/trunk/lib/Core/Resolver.cpp
>>>
>>> Modified: lld/trunk/lib/Core/Resolver.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Core/Resolver.cpp?rev=181492&r1=181491&r2=181492&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/lib/Core/Resolver.cpp (original)
>>> +++ lld/trunk/lib/Core/Resolver.cpp Wed May  8 18:54:10 2013
>>> @@ -200,7 +200,11 @@ void Resolver::resolveUndefines() {
>>>        StringRef undefName = undefAtom->name();
>>>        // load for previous undefine may also have loaded this undefine
>>>        if (!_symbolTable.isDefined(undefName)) {
>>> -        _inputFiles.searchLibraries(undefName, true, true, false,
>>> *this);
>>> +        _inputFiles.searchLibraries(undefName,
>>> +                                    true,   // searchSharedLibs
>>> +                                    true,   // searchArchives
>>> +                                    false,  // dataSymbolOnly
>>> +                                    *this);
>>>        }
>>>      }
>>>      // search libraries for overrides of common symbols
>>> @@ -213,10 +217,13 @@ void Resolver::resolveUndefines() {
>>>          const Atom *curAtom = _symbolTable.findByName(tentDefName);
>>>          assert(curAtom != nullptr);
>>>          if (const DefinedAtom* curDefAtom =
>>> dyn_cast<DefinedAtom>(curAtom)) {
>>> -          if (curDefAtom->merge() == DefinedAtom::mergeAsTentative ) {
>>> +          if (curDefAtom->merge() == DefinedAtom::mergeAsTentative) {
>>>              // Still tentative definition, so look for override.
>>> -            _inputFiles.searchLibraries(tentDefName, searchSharedLibs,
>>> -                                        searchArchives, true, *this);
>>> +            _inputFiles.searchLibraries(tentDefName,
>>> +                                        searchSharedLibs,
>>> +                                        searchArchives,
>>> +                                        true,  // dataSymbolOnly
>>> +                                        *this);
>>>            }
>>>          }
>>>        }
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130508/1e3f8308/attachment.html>