diff mbox

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

Message ID CABOHX+dAvESSm0gHL4vvwX4JuoZhiy+uac6hSirQ6c35Da0_SQ@mail.gmail.com
State New, archived
Headers

Commit Message

Iain Buclaw Oct. 11, 2015, 12:01 p.m. UTC 
  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.
---

Comments

Pedro Alves Oct. 19, 2015, 3:42 p.m. UTC | #1 
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
Iain Buclaw Oct. 23, 2015, 8:35 p.m. UTC | #2 
On 19 October 2015 at 17:42, Pedro Alves <palves@redhat.com> 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
Doug Evans Oct. 26, 2015, 12:17 a.m. UTC | #3 
> On 19 October 2015 at 17:42, Pedro Alves <palves@redhat.com> 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.
Pedro Alves Oct. 26, 2015, 4:30 p.m. UTC | #4 
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
Iain Buclaw Oct. 29, 2015, 3:28 p.m. UTC | #5 
On 26 October 2015 at 17:30, Pedro Alves <palves@redhat.com> 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.
Iain Buclaw Oct. 29, 2015, 3:32 p.m. UTC | #6 
On 26 October 2015 at 01:17, Doug Evans <xdje42@gmail.com> wrote:
>> On 19 October 2015 at 17:42, Pedro Alves <palves@redhat.com> 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.
Iain Buclaw Jan. 30, 2016, 1:50 p.m. UTC | #7 
On 29 October 2015 at 16:32, Iain Buclaw <ibuclaw@gdcproject.org> wrote:
> On 26 October 2015 at 01:17, Doug Evans <xdje42@gmail.com> wrote:
>>> On 19 October 2015 at 17:42, Pedro Alves <palves@redhat.com> 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.
diff mbox

Patch

gdb/ChangeLog:

	* d-namespace.c (d_lookup_symbol_imports): Remove argument
	'search_parents'.  All callers updated.

---
diff --git a/gdb/d-namespace.c b/gdb/d-namespace.c
index da5da58..1524047 100644
--- a/gdb/d-namespace.c
+++ b/gdb/d-namespace.c
@@ -388,31 +388,15 @@  reset_directive_searched (void *data)
 }
 
 /* Search for NAME by applying all import statements belonging to
-   BLOCK which are applicable in SCOPE.
-
-   If SEARCH_PARENTS the search will include imports which are
-   applicable in parents of SCOPE.
-   Example:
-
-     module A;
-     import X;
-     void B() {
-       import Y;
-     }
-
-   If SCOPE is "A.B" and SEARCH_PARENTS is true, the imports of
-   modules X and Y will be considered.  If SEARCH_PARENTS is false
-   only the import of Y is considered.  */
+   BLOCK which are applicable in SCOPE.  */
 
 static struct block_symbol
 d_lookup_symbol_imports (const char *scope, const char *name,
 			 const struct block *block,
-			 const domain_enum domain,
-			 const int search_parents)
+			 const domain_enum domain)
 {
   struct using_direct *current;
   struct block_symbol sym;
-  int directive_match;
   struct cleanup *searched_cleanup;
 
   /* First, try to find the symbol in the given module.  */
@@ -430,18 +414,9 @@  d_lookup_symbol_imports (const char *scope, const char *name,
        current = current->next)
     {
       const char **excludep;
-      int len = strlen (current->import_dest);
-
-      directive_match = (search_parents
-			 ? (strncmp (scope, current->import_dest, len) == 0
-			    && (len == 0
-				|| scope[len] == '.'
-				|| scope[len] == '\0'))
-			 : strcmp (scope, current->import_dest) == 0);
-
-      /* If the import destination is the current scope or one of its
-	 ancestors then it is applicable.  */
-      if (directive_match && !current->searched)
+
+      /* If the import destination is the current scope then search it.  */
+      if (!current->searched && strcmp (scope, current->import_dest) == 0)
 	{
 	  /* Mark this import as searched so that the recursive call
 	     does not search it again.  */
@@ -554,7 +529,7 @@  d_lookup_symbol_module (const char *scope, const char *name,
      blocks.  */
   while (block != NULL)
     {
-      sym = d_lookup_symbol_imports (scope, name, block, domain, 1);
+      sym = d_lookup_symbol_imports (scope, name, block, domain);
 
       if (sym.symbol != NULL)
 	return sym;