OpenEuphoria

Euphoria Ticket #169: find_nested -1 bad choice for default no routine_id

• Reported by ne1uno Jun 11, 2010
• 6 comments

 find_nested(object needle, sequence haystack, integer flags=0, integer rtn_id=-1) 
 
 function fnfind(object needle, object haystack) 
 	return find(needle, haystack) 
 end function 
 
   test_equal("lookup4c-1", {{3,2}, {3,1}, {2}}  , 
	find_nested({3, 2}, {1, 3, {2,3}}, NESTED_ANY + 
                 NESTED_BACKWARD + NESTED_ALL, routine_id("ffind")) 
  ) 
--woops, meant to type routine_id("fnfind") 
 

Details

1. Comment by jimcbrown Jun 12, 2010

Suggests to me that you are saying that we should make the function smart enough to crash if passed in a routine id of -1 (as this is due to a user error of having a typo in a routine_id() or something of a similiar issue) and then use a different value (such as -2, -3, etc) to represent NO_ROUTINE_ID.

In that case I'd prefer having a public constant NO_ROUTINE_ID = -99999 or similar, and then using the constant instead of having to remember that -2 is hardcoded to act this way.

Then make the defaulted value NO_ROUTINE_ID.

Maybe also public constant INVALID_ROUTINE_ID = -1 ?

2. Comment by DerekParnell Jun 12, 2010

I'm thinking along the same lines as Jim, and to be consistent we'd need to apply the same constants and logic to all places where a standard library routine accepts a routine_id.

3. Comment by ne1uno Jun 14, 2010

• constants NO_ROUTINE_ID and INVALID_ROUTINE_ID added to types.e

• find_nested changed to use NO_ROUTINE_ID in search.e
• walk_dir changed to use NO_ROUTINE_ID in filesys.e

• cmdline.e uses -1 extensively. much of the docs, examples and existing code will also have used -1 so I didn't change anything. the worse that can happen from the default fallback in case of user error on invalid routine_id. would be how or what to display as commandline help. should be easy enough to discover that a user defined routine is not getting called and why with minimal testing.

another questionable/dual use of routine_id that I see is in filter, where the same var is used in a switch statement comparing many format strings defaulting case else to use as a routine_id directly. I will be surprised if there are no combinations of usage that will not expose a bug, most surely if translated. since tests are passing maybe there is no problem?

4. Comment by ne1uno Jun 17, 2010

spoke too soon, cmdline.e opts can also call a function and this useage has -1 as a default.

what about zero? can that be reserved as never a valid routine_id? that would solve a few problems. there are already more than enough specialized enum constants detailing various ways of setting up options. -1 should be checked for while building the options rather than crashing the user who through no fault of their own finds a problem that could have been caught earlier. I realize there is a limit to how cmdline will be able to deal with a programmer error that fairly simple testing could have caught. it should be possible to produce a warning rather than a crash or wrong interpretation of -1.

if zero isn't an option, how about anything -9 or less will mean no routine_id, giving those who would rather not include types.e an easy constant to use and would visually signify no_routine_id the way -1 now does.

changed to feature request for zero to be a reserved routine_id if it isn't already.

5. Comment by DerekParnell Jun 17, 2010

The value zero is NOT a good choice as a default because that is a valid routine_id. I'm in favour of using -2 as the defined NO_ROUTINE_ID but making sure the test for it is if parm <= NO_ROUTINE_ID then -- no routine passed

6. Comment by jimcbrown Nov 09, 2010

Reserving 0 and not allowing it as a routine id isn't hard, but the consensus seems to be that it isn't necessary.

cmdline.e probably should have been changed to the NO_ROUTINE_ID / INVALID_ROUTE_ID scheme, but it is too late to do anything about that now.

Changing back to Bug Report and marking as fixed (as the original walk_dir() and friends were already fixed to use this scheme).

Include: