String Type Helper Join lazarus / 64-FPC development ---------------------------------------------------------------------- WayneSherman · 2022-05-01 15:36 I was trying to join strings using helpers and discovered that the value of the string is completely ignored: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i AString := 'C:';//none of these work correctlyAString.Join('\', ['Windows', 'System32']);  //this doesn't do anythingAString := AString.Join('\', ['Windows', 'System32']); //this misses the 'C:', i.e. the contents of AString is ignoredAString := AString + AString.Join('\', ['Windows','System32']); //this doesn't use the separator on the first concatentation. //This works, but uglyAString := 'C:';AString := AString + '\' + AString.Join('\', ['Windows', 'System32']); //or maybe this one is the best wayAString := AString.Join('\', [AString, 'Windows', 'System32']); Also, it would be nice to have a "PathJoin" helper which automatically uses the correct OS path separator and normalizes the path to remove double separators and fix wrong separators: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i AString := 'C:\Windows\';AString := AString.PathJoin(['\system32\', '/drivers/', 'etc', 'hosts']);// on Windows gives 'C:\Windows\system32\drivers\etc\hosts'// on Linux gives 'C:/Windows/system32/drivers/etc/hosts' ---------------------------------------------------------------------- howardpc · 2022-05-01 16:13 Quote from: WayneSherman on May 01, 2022, 05:36:00 pm//none of these work correctly AString.Join('\', ['Windows', 'System32']);  //this doesn't do anything Your assertion is not correct. It is just that your statement throws away the result of the function. ---------------------------------------------------------------------- marcov · 2022-05-01 16:17 Delphi has a TPath type, I think, for path strings. ---------------------------------------------------------------------- Laprado · 2022-05-01 23:08 To use join function: AString := string.Join('\', ['c:', 'Windows', 'System32']); For path you may use ConcatPaths from SysUtils unit: APath := ConcatPaths(['c:', 'Windows', 'System32']); ---------------------------------------------------------------------- trev · 2022-05-01 23:38 Quote from: WayneSherman on May 01, 2022, 05:36:00 pmAlso, it would be nice to have a "PathJoin" helper which automatically uses the correct OS path separator and normalizes the path to remove double separators and fix wrong separators You are using literal path separators! Instead, use the PathDelim constant. ---------------------------------------------------------------------- dbannon · 2022-05-02 00:01 I personally consider this far more readable, simple and easy to spot mistakes - Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i AString := PathDelim + 'system32' + PathDelim + 'drivers' + PathDelim + 'etc' + PathDelim + 'hosts'; It could be improved by using macro to define a shorter name for PathDelim but you get used to it as it is. Davo ---------------------------------------------------------------------- WayneSherman · 2022-05-02 16:32 I don't understand the choice to use a class function when working on a string instance is an obvious use case.  For example, I find it misleading that this does not result in 'abcdef': Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i 'abc'.Join('', ['def']); Likewise, this does not work as expected: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i   TemplateDir := Application.Location.Join(PathDelim, ['templates']); ---------------------------------------------------------------------- dbannon · 2022-05-03 00:30 Wayne, it does not work as you expect, that is not necessarily a bug. Consider this - Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i var    LeftBit : string = 'left';    RightBit : string = 'right';    MyString : string = 'abc';begin    MyString := MyString.Join(PathDelim, [LeftBit, MyString, RightBit]);    // MyString is now 'left/abc/right'  If Join automatically inserted its own value into the set, as you seem to expect, the code above would give a different result. But if that was the case, would you expect it to add it as the first item in the set, the last item in the set or the middle item (where I want it) ? Davo ---------------------------------------------------------------------- trev · 2022-05-03 01:24 Perhaps reading the documentation would help to understand how it works rather than imagining how it should work. ---------------------------------------------------------------------- WayneSherman · 2022-05-03 04:38 Quote from: dbannon on May 03, 2022, 02:30:52 amWayne, it does not work as you expect, that is not necessarily a bug. Yes, I agree it is not a bug.  I just find it not intuitive when Lazarus code completion recommends helpers that (in my opinion) should not be showing up on a string instance since those functions don't operate on it. Quote MyString := MyString.Join(PathDelim, [LeftBit, MyString, RightBit]);     // MyString is now 'left/abc/right' If Join automatically inserted its own value into the set, as you seem to expect, the code above would give a different result. But if that was the case, would you expect it to add it as the first item in the set, the last item in the set or the middle item (where I want it) If MyString has a value when Join is called, I would expect it to be the first string (i.e. the left most string; typically a concatenate adds to the end of a string).  If it operated on string instances, to insert it into an arbitrary position you would have to: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i MyString := ''.Join(PathDelim, [LeftBit, MyString, RightBit]);//EDIT: nope, that won't do it, I forgot we have a delimiter which//would end up at the start of the string. it would have to be like this:MyString := LeftBit.Join(PathDelim, [MyString, RightBit]); For clarity and to avoid confusion, a convention is now needed such that the Join helper should never be called on a string instance, but always on the type directly: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i MyString := String.Join(...);  //this convention is neededMyString := SomeString.Join(...);  //never write code like thisMyString := 'a string literal'.Join(...);  //never write code like this ---------------------------------------------------------------------- Zvoni · 2022-05-03 06:59 To add a bit of spice (or call it personal preference): What confused me the most is the fact, that a "Join" is usually done on an Array (which the Syntax actually implies! --> ...Join(Delim, [ArrMember1, ArrMember2....]) So IMO it would make more sense to have something like Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i MyString:=MyArray.Join(....)But Typehelpers for Arrays is a different can of worms, never mind existing code would break ---------------------------------------------------------------------- PascalDragon · 2022-05-03 07:35 Quote from: WayneSherman on May 03, 2022, 06:38:19 amQuote from: dbannon on May 03, 2022, 02:30:52 amWayne, it does not work as you expect, that is not necessarily a bug. Yes, I agree it is not a bug.  I just find it not intuitive when Lazarus code completion recommends helpers that (in my opinion) should not be showing up on a string instance since those functions don't operate on it. This is consistent with the remainder of the language: You can call ordinary class methods and static class functions on an instance as well, helper types make no exception here. It's up to the user to call them correctly. ---------------------------------------------------------------------- MarkMLl · 2022-05-03 08:36 Quote from: WayneSherman on May 03, 2022, 06:38:19 amYes, I agree it is not a bug.  I just find it not intuitive when Lazarus code completion recommends helpers that (in my opinion) should not be showing up on a string instance since those functions don't operate on it. That's a Lazarus IDE issue and should be discussed (and improvements contributed) elsewhere. The only possible way that that would impact on FPC as a language and on the RTL/FCL would be if the Lazarus team requested that function source be annotated to allow the IDE to improve its completion suggestions. MarkMLl ---------------------------------------------------------------------- PascalDragon · 2022-05-03 11:29 Quote from: MarkMLl on May 03, 2022, 10:36:01 amQuote from: WayneSherman on May 03, 2022, 06:38:19 amYes, I agree it is not a bug.  I just find it not intuitive when Lazarus code completion recommends helpers that (in my opinion) should not be showing up on a string instance since those functions don't operate on it. That's a Lazarus IDE issue and should be discussed (and improvements contributed) elsewhere. The only possible way that that would impact on FPC as a language and on the RTL/FCL would be if the Lazarus team requested that function source be annotated to allow the IDE to improve its completion suggestions. Nothing needs to be annotated there, because the method is marked as class. Maybe Lazarus then needs to improve its completion display, because one can't easily differentiate between a class and an instance method. However calling a class method on an instance is valid code and thus the suggestion of that function by itself is valid as well. ---------------------------------------------------------------------- MarkMLl · 2022-05-03 12:21 Quote from: PascalDragon on May 03, 2022, 01:29:28 pmNothing needs to be annotated there, because the method is marked as class. Maybe Lazarus then needs to improve its completion display, because one can't easily differentiate between a class and an instance method. However calling a class method on an instance is valid code and thus the suggestion of that function by itself is valid as well. Agreed. I was pointing out the one potential situation which might conceivably make this an FPC issue: as things stand it's not. And even if it were booted back to the FPC project, I'd expect it to be in the form of a patch asking that %directives be added to the RTC or FCL, i.e. it would be here purely because source needed annotating rather than because anybody was asking that FPC's behaviour or Object Pascal's syntax be changed. MarkMLl ---------------------------------------------------------------------- avk · 2022-05-03 12:54 To be honest, I would be more worried about the details of its implementation(meaning AnsiString.Join()). It turns out that the function can raise an exception, which the documentation is modestly silent about. But is an exception really necessary? Especially considering that, moreover, not all unwanted cases are covered? For example, System.Copy() just returns an empty string in such cases. And it's sad to see such ugliness in a function from RTL as allocating memory inside a loop. ---------------------------------------------------------------------- WayneSherman · 2022-05-03 14:30 Quote from: WayneSherman on May 03, 2022, 06:38:19 amFor clarity and to avoid confusion, a convention is now needed such that the Join helper should never be called on a string instance, but always on the type directly: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i MyString := String.Join(...);  //Always use Join on the type directly like thisMyString := SomeString.Join(...);   //never write code like thisMyString := 'a string literal'.Join(...);  //never write code like this For type helpers, if it is bad practice to call a class function on an instance of the type, would it be better to:   - Enforce good form by generating a compile error (or at least a warning)?   - For Lazarus code completion, do not show class functions on a instance of the type? ---------------------------------------------------------------------- PascalDragon · 2022-05-04 07:29 Quote from: WayneSherman on May 03, 2022, 04:30:23 pmQuote from: WayneSherman on May 03, 2022, 06:38:19 amFor clarity and to avoid confusion, a convention is now needed such that the Join helper should never be called on a string instance, but always on the type directly: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i MyString := String.Join(...);  //Always use Join on the type directly like thisMyString := SomeString.Join(...);   //never write code like thisMyString := 'a string literal'.Join(...);  //never write code like this For type helpers, if it is bad practice to call a class function on an instance of the type, would it be better to:   - Enforce good form by generating a compile error (or at least a warning)?   - For Lazarus code completion, do not show class functions on a instance of the type? Again: it is valid code. It makes no sense enforce a compile error or even a warning for valid code. Also it makes no sense for Lazarus to hide the methods because it is valid code. What Lazarus should do instead (and which would help in other situations as well) is to highlight that these are class methods, because Lazarus currently doesn't do that. ---------------------------------------------------------------------- avk · 2022-05-05 11:34 I've added some version of Join() to my TAnsiStrHelper(Join2), which does not raise exceptions and seems to be about three times faster than the RTL version. Does it make sense to offer it as a replacement for the current RTL version, or let it stay in TAnsiStrHelper? ---------------------------------------------------------------------- Warfley · 2022-05-05 15:40 Quote from: dbannon on May 02, 2022, 02:01:29 amI personally consider this far more readable, simple and easy to spot mistakes - Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i AString := PathDelim + 'system32' + PathDelim + 'drivers' + PathDelim + 'etc' + PathDelim + 'hosts'; It could be improved by using macro to define a shorter name for PathDelim but you get used to it as it is. Davo I've created a TPath class that allows the following: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i AString := TPath('C:') / 'system32' / 'drivers' / 'etc' / 'hosts';But currently the normalize function is still quite buggy and I hadn't had time to fix it (basically resolving '..' at the start of the path doesn't work so 'foo/../../bar' should be normalized to '../bar' but this doesn't work right now). ---------------------------------------------------------------------- PascalDragon · 2022-05-06 13:50 Quote from: avk on May 05, 2022, 01:34:32 pmI've added some version of Join() to my TAnsiStrHelper(Join2), which does not raise exceptions and seems to be about three times faster than the RTL version. Does it make sense to offer it as a replacement for the current RTL version, or let it stay in TAnsiStrHelper? If Delphi raises exceptions as well, then no, it won't be a suitable replacement. Otherwise: simply report it. If suitable someone (probably MvC) will pick it up or reject it. ---------------------------------------------------------------------- avk · 2022-05-06 16:11 Thank you. Fortunately, a very similar proposal has already been presented there.