[PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Iain Buclaw
Whilst looking at part one, a moment of insight came to me and I
realized this code is completely nonsensical.

For a start, when importing modules, you don't gain access to all
parent packages of the given module.

To add some confusion, even the comment was wrong.  It doesn't even
cater for the example given (it's d_lookup_symbol_module that walks up
each block scope).

I feel embarrassed it didn't come to me before. :-)

Regards,
Iain.
---

dlang-searchparents.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Pedro Alves-7
On 10/11/2015 01:01 PM, Iain Buclaw wrote:

> Whilst looking at part one, a moment of insight came to me and I
> realized this code is completely nonsensical.
>
> For a start, when importing modules, you don't gain access to all
> parent packages of the given module.
>
> To add some confusion, even the comment was wrong.  It doesn't even
> cater for the example given (it's d_lookup_symbol_module that walks up
> each block scope).
>
> I feel embarrassed it didn't come to me before. :-)

The usual penance is writing test cases.  :-)

Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Iain Buclaw
On 19 October 2015 at 17:42, Pedro Alves <[hidden email]> wrote:

>
> On 10/11/2015 01:01 PM, Iain Buclaw wrote:
> > Whilst looking at part one, a moment of insight came to me and I
> > realized this code is completely nonsensical.
> >
> > For a start, when importing modules, you don't gain access to all
> > parent packages of the given module.
> >
> > To add some confusion, even the comment was wrong.  It doesn't even
> > cater for the example given (it's d_lookup_symbol_module that walks up
> > each block scope).
> >
> > I feel embarrassed it didn't come to me before. :-)
>
> The usual penance is writing test cases.  :-)
>

It helps if there is a compiler readily available to compile said
tests.  However, there likely is a way to get around this that I'm not
aware of. (Skip certain tests if a compiler doesn't exist?  ;-)

With this patch though, it's all dead code.  Hard to write a test for
something that is unreachable.

Regards
Iain
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Doug Evans-5
> On 19 October 2015 at 17:42, Pedro Alves <[hidden email]> wrote:
>>
>> On 10/11/2015 01:01 PM, Iain Buclaw wrote:
>> > Whilst looking at part one, a moment of insight came to me and I
>> > realized this code is completely nonsensical.
>> >
>> > For a start, when importing modules, you don't gain access to all
>> > parent packages of the given module.
>> >
>> > To add some confusion, even the comment was wrong.  It doesn't even
>> > cater for the example given (it's d_lookup_symbol_module that walks up
>> > each block scope).
>> >
>> > I feel embarrassed it didn't come to me before. :-)
>>
>> The usual penance is writing test cases.  :-)
>>
>
> It helps if there is a compiler readily available to compile said
> tests.  However, there likely is a way to get around this that I'm not
> aware of. (Skip certain tests if a compiler doesn't exist?  ;-)
>
> With this patch though, it's all dead code.  Hard to write a test for
> something that is unreachable.

Would the testsuite's DWARF assembler help here?
IOW, write the test in DWARF, not D.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Pedro Alves-7
In reply to this post by Iain Buclaw
On 10/23/2015 09:35 PM, Iain Buclaw wrote:

> It helps if there is a compiler readily available to compile said
> tests.  However, there likely is a way to get around this that I'm not
> aware of. (Skip certain tests if a compiler doesn't exist?  ;-)

The C, C++, Go, Java, Ada, Fortran and Pascal, etc. tests do get skipped if a
compiler is not available.  I'd hazard a guess that you're missing a
compiler for at least one of those languages.  :-)

If you hook D program building through
prepare_for_testing/gdb_compile/default_target_compile etc., then D
tests will behave the same.  Basically, tweak lib/future.exp:gdb_default_target_compile
to understand "d" (and send the equivalent patch to DejaGnu).

See a766d390 for when "go" support was added.

Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Iain Buclaw
On 26 October 2015 at 17:30, Pedro Alves <[hidden email]> wrote:

> On 10/23/2015 09:35 PM, Iain Buclaw wrote:
>
>> It helps if there is a compiler readily available to compile said
>> tests.  However, there likely is a way to get around this that I'm not
>> aware of. (Skip certain tests if a compiler doesn't exist?  ;-)
>
> The C, C++, Go, Java, Ada, Fortran and Pascal, etc. tests do get skipped if a
> compiler is not available.  I'd hazard a guess that you're missing a
> compiler for at least one of those languages.  :-)
>
> If you hook D program building through
> prepare_for_testing/gdb_compile/default_target_compile etc., then D
> tests will behave the same.  Basically, tweak lib/future.exp:gdb_default_target_compile
> to understand "d" (and send the equivalent patch to DejaGnu).
>
> See a766d390 for when "go" support was added.
>
> Thanks,
> Pedro Alves
>

I've made a note and will explore that avenue.  Thanks!

Iain.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Iain Buclaw
In reply to this post by Doug Evans-5
On 26 October 2015 at 01:17, Doug Evans <[hidden email]> wrote:

>> On 19 October 2015 at 17:42, Pedro Alves <[hidden email]> wrote:
>>>
>>> On 10/11/2015 01:01 PM, Iain Buclaw wrote:
>>> > Whilst looking at part one, a moment of insight came to me and I
>>> > realized this code is completely nonsensical.
>>> >
>>> > For a start, when importing modules, you don't gain access to all
>>> > parent packages of the given module.
>>> >
>>> > To add some confusion, even the comment was wrong.  It doesn't even
>>> > cater for the example given (it's d_lookup_symbol_module that walks up
>>> > each block scope).
>>> >
>>> > I feel embarrassed it didn't come to me before. :-)
>>>
>>> The usual penance is writing test cases.  :-)
>>>
>>
>> It helps if there is a compiler readily available to compile said
>> tests.  However, there likely is a way to get around this that I'm not
>> aware of. (Skip certain tests if a compiler doesn't exist?  ;-)
>>
>> With this patch though, it's all dead code.  Hard to write a test for
>> something that is unreachable.
>
> Would the testsuite's DWARF assembler help here?
> IOW, write the test in DWARF, not D.

Yes, that too, it's just a process that I can foresee taking a while
to get right.

Iain.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] [D] Remove search_parents parameter from d_lookup_symbol_imports

Iain Buclaw
On 29 October 2015 at 16:32, Iain Buclaw <[hidden email]> wrote:

> On 26 October 2015 at 01:17, Doug Evans <[hidden email]> wrote:
>>> On 19 October 2015 at 17:42, Pedro Alves <[hidden email]> wrote:
>>>>
>>>> On 10/11/2015 01:01 PM, Iain Buclaw wrote:
>>>> > Whilst looking at part one, a moment of insight came to me and I
>>>> > realized this code is completely nonsensical.
>>>> >
>>>> > For a start, when importing modules, you don't gain access to all
>>>> > parent packages of the given module.
>>>> >
>>>> > To add some confusion, even the comment was wrong.  It doesn't even
>>>> > cater for the example given (it's d_lookup_symbol_module that walks up
>>>> > each block scope).
>>>> >
>>>> > I feel embarrassed it didn't come to me before. :-)
>>>>
>>>> The usual penance is writing test cases.  :-)
>>>>
>>>
>>> It helps if there is a compiler readily available to compile said
>>> tests.  However, there likely is a way to get around this that I'm not
>>> aware of. (Skip certain tests if a compiler doesn't exist?  ;-)
>>>
>>> With this patch though, it's all dead code.  Hard to write a test for
>>> something that is unreachable.
>>
>> Would the testsuite's DWARF assembler help here?
>> IOW, write the test in DWARF, not D.
>
> Yes, that too, it's just a process that I can foresee taking a while
> to get right.
>
> Iain.

[Apologies for the necromancing]

I've managed to finally get round to producing a reduced DWARF test
for 1/2 in this series.  However I'm not able to produce one for this,
as it's just a refactor of dead code.

Iain.