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

[Rui Ueyama]

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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130508/c1ecb859/attachment.html>