Modify

Opened 7 years ago

Closed 7 years ago

#2174 closed Bug (Completed)

Code error in _StringInsert() - but in my opinion it should not be

Reported by: amarcruz Owned by: guinness
Milestone: 3.3.9.5 Component: Standard UDFs
Version: 3.3.8.1 Severity: None
Keywords: Standard UDFs, _StringInsert Cc:

Description

In Line# 270 of String.au3 (6 of _StringInsert())

ElseIf $s_InsertString = "" Or (Not IsString($s_String)) Then

should be read:

ElseIf $s_InsertString = "" Or (Not IsString($s_InsertString)) Then

by the way,
like this, many functions in Standard UDFs check its parameter type; this is NOT CONSISTENT with AutoIt philosophy, I think.

AutoIt allows expressions like this:
$x = 123 & 456 ; x="123456"
with implicit type convertion, no error.

Why _StringInsert("123", 456, 3) should not working and set @error?
Why _StringInsert("123", "", 3) set @error?
...should not.

OK, this is my opinion, of course.
I have a 17 years base in Assembler, C, FoxPro/VFP, HTML, Javascript, and VBScript, yet I may be wrong.

...and Sorry for my horrible "English" :)

Attachments (1)

StringInsertNEW.au3 (1.1 KB) - added by amarcruz 7 years ago.
_StringInsert, corrected & minor improved version

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by amarcruz

_StringInsert, corrected & minor improved version

comment:1 Changed 7 years ago by anonymous

I forgot, another inconsitence:
in _StringInsert, $i_position parameter is base 0, not 1.

Help don't say that.

comment:2 Changed 7 years ago by BrewManNH

Why _StringInsert("123", 456, 3) should not working and set @error?
Why _StringInsert("123", "", 3) set @error?

The first one works without any errors for me. Are you saying it should or it shouldn't give you an error?

The second one doesn't make a lot of sense, "" may be regarded as a string, but why would you ever try to insert a string with nothing in it? I think that if the string is sent as "", it should be reported as an error because the length of the string is 0, which means you sent the function nothing to work with. Also, if you send it the source string as "" you're not adding anything to an existing string you might as well just use the second parameter as the string, because that's what you'd get back if it worked as you wanted it to.

comment:3 Changed 7 years ago by jchd

Inserting an empty string should definitely NOT set @error. An empty string is not "nothing to qork with" but a perfectly legitimate string. That's IMHO.
I'd even would rather the function be:

; #FUNCTION# ====================================================================================================================
; Name...........: __StringInsert
; Description ...: Inserts a string within another string.
; Syntax.........: __StringInsert($s_String, $s_InsertString, $i_Position)
; Parameters ....: $s_String   - Original string
;                  $s_InsertString   - String to be inserted
;                  $i_Position - Position to insert string (negatives values count from right hand side)
; Return values .: Success - Returns new modified string
;                  Failure - Returns original string and sets the @error to the following values:
;                  |@error = 3 : Invalid position
; Author ........: Louis Horvath <celeri at videotron dot ca>
; Modified.......: jchd
; Remarks .......: Use negative position values to insert string from the right hand side.
; Related .......:
; Link ..........:
; Example .......: Yes
; ===============================================================================================================================
Func __StringInsert($s_String, $s_InsertString, $i_Position)
	$s_String = String($s_String)
	$i_Position = Int($i_Position)
	Local $i_Length = StringLen($s_String) ; Take a note of the length of the source string
	If Abs($i_Position) > $i_Length Then
		Return SetError(3, 0, $s_String) ; Invalid position
	EndIf

	If $i_Position >= 0 Then
		Return(StringRegExpReplace($s_String, "(?s)\A(.{" & $i_Position & "})(.*)\z", "${1}" & String($s_InsertString) & "$2"))
	Else
		Return(StringRegExpReplace($s_String, "(?s)\A(.*)(.{" & -$i_Position & "})\z", "${1}" & String($s_InsertString) & "$2"))
	EndIf
EndFunc   ;==>__StringInsert

I find this much more consistent with generic AutoIt expressions using Variant "variance" (e.g. like True & "abc" & 456 which doesn't bark about True and 456 not being strings).

comment:4 Changed 7 years ago by BrewManNH

How do you insert nothing (e.g. "") into a string? If it sets @error and tells you that the string was empty, then you can use error checking to see that your insert was empty and the result is the original string. This will at least tell the coder that they are attempting something that didn't work as expected because their string was empty. I can see having it accept an empty source string, but I don't see why you'd want an empty insert string to be legitimate. Differing opinions I suppose, but with @error, you're getting feedback that something went wrong.

comment:5 Changed 7 years ago by Valik

Should we modify AutoIt so that the following generates a syntax error?

Local $s = "abc" & "" & "def"

That would be stupid, right? So why then should _StringInsert() treat inserting an empty string as an error? It's a harmless - albeit pointless - operation.

comment:6 Changed 7 years ago by jchd

True, since in many cases, inserted string comes as a programatic result which may be the (perfectly valid) empty string, e.g. like a partial result from a regexp where some capturing group may capture nothing. I see no "error" here nor any reason to force complication in the code, only convenience.

If we start flagging all identity operations like $c = $a * $b or $i += $j where $b may be 1 and $j may be zero, we end up with a useless language in practice (IMVHO).

$s = "abc"
$s &= (1 = 2)
gives abcFalse and _StringInsert has no reason to behave differently.

comment:7 Changed 7 years ago by amarcruz

Valik, jchd, I agree.
_StringInsert(123, 456, 3) should return "123456"
This is consistent with Autoit.

comment:8 Changed 7 years ago by guinness

  • Milestone set to 3.3.9.5
  • Owner set to guinness
  • Resolution set to Completed
  • Status changed from new to closed

Removed by revision [7230] in version: 3.3.9.5

Guidelines for posting comments:

  • You cannot re-open a ticket but you may still leave a comment if you have additional information to add.
  • In-depth discussions should take place on the forum.

For more information see the full version of the ticket guidelines here.

Add Comment

Modify Ticket

Action
as closed The owner will remain guinness.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.