Project

General

Profile

Bug #7604

Switch does not honor case sensitivity of character

Added by Eduard Soltan 12 months ago. Updated 10 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:

7604_switch_substring.diff Magnifier (747 Bytes) Eduard Soltan, 07/20/2023 09:59 AM

7604_substring_parameter_check.diff Magnifier (1015 Bytes) Eduard Soltan, 07/21/2023 02:41 AM

History

#1 Updated by Eduard Soltan 12 months ago

case substring("IKI", 1, 2, "character":U):
  when 'ik':U then
    message "Hello1".
  when 'vk':U then
    message "Hello2".
  when 'ih':U then
    message "Hello3".
  when 'vh':U then
    message "Hello4".
end case.

Code in OE get converted in;

switch (StringHelper.safeTrimTrailing((substring("IKI", 1, 2, "character")).toStringMessage()))
{
  case "ik":
  {
     message("Hello1");
  }
  break;
  case "vk":
  {
     message("Hello2");
  }
  break;
  case "ih":
  {
     message("Hello3");
  }
  break;
  case "vh":
  {
     message("Hello4");
  }
  break;
}

The following code in OE displays Hello1 on the screen, but in FWD it does not enter any switch cases.

#2 Updated by Eduard Soltan 12 months ago

Tested switch with following cases:

- character variable case-insensitive
- character variable case-sensitive
- entry function on character variable case-insensitive
- entry function on character variable case-sensitive
- trim function on character variable case-insensitive
- trim function on character variable case-sensitive
- substring function on character variable case-sensitive

In all cases fwd works as expected. The only problem appears at using substring function with character variable with case-insensitive.

#3 Updated by Greg Shah 12 months ago

Please go ahead and fix this.

#4 Updated by Alexandru Lungu 12 months ago

  • Status changed from New to WIP
  • Assignee set to Eduard Soltan

#5 Updated by Eduard Soltan 12 months ago

case FUNC_CHAR:
case PLUS:
  for (int k = 0; k < ast.getNumImmediateChildren(); k++) 
  {
     if (!isCaseInsensitive(ast.getChildAt(k)))
     {
        // if a child is found to be case-sensitive then the result is case-sensitive
        return false;
     }
  }

offset and length paramentes from substring function, isCaseInsensitive method for ExpressionConversionWorker.java class returns false, because they are of integer type.

#6 Updated by Alexandru Lungu 12 months ago

  • % Done changed from 0 to 50

offset and length paramentes from substring function, isCaseInsensitive method for ExpressionConversionWorker.java class returns false, because they are of integer type.

This is quite "cryptic" :) Eduard, please make some second/third rechecks before Redmine posts. Also, please explain the relevance of the snippet in regard to the changes. As far as I could "decrypt": SUBSTRING has 3 parameters, from which the second (offset) and the third (length) are integer. Therefore, the for-loop in the snippet is returning false for sub-string, because integer data-type causes !isCaseInsensitive(ast.getChildAt(k)) to evaluate to true, so the switch is case-sensitive. This is not right for SUBSTRING, as the case-sensitivity depends only on the first character parameter.

Is the diff working for your case? It is quite weird that you use SUBSTRING, but made changes to an IF with ENTRY. I expected to more like:

+               if (ast.getText().equalsIgnoreCase(/*"ENTRY"*/ "SUBSTRING") && isCaseInsensitive(ast.getChildAt(0))) 
+               {
+                  return true;
+               }

Also, can you make a special case for "SUBSTRING" entirely:

+               if (ast.getText().equalsIgnoreCase("SUBSTRING")) 
+               {
+                  return isCaseInsensitive(ast.getChildAt(0));
+               }

Is this correct? Please make some tests similar to #7604-2 and ensure nothing is broken + the issue is fixed.

#7 Updated by Eduard Soltan 12 months ago

Alexandru Lungu wrote:

This is quite "cryptic" :) Eduard, please make some second/third rechecks before Redmine posts. Also, please explain the relevance of the snippet in regard to the changes. As far as I could "decrypt": SUBSTRING has 3 parameters, from which the second (offset) and the third (length) are integer. Therefore, the for-loop in the snippet is returning false for sub-string, because integer data-type causes !isCaseInsensitive(ast.getChildAt(k)) to evaluate to true, so the switch is case-sensitive. This is not right for SUBSTRING, as the case-sensitivity depends only on the first character parameter.

Is the diff working for your case? It is quite weird that you use SUBSTRING, but made changes to an IF with ENTRY. I expected to more like:

+               if (ast.getText().equalsIgnoreCase(/*"ENTRY"*/ "SUBSTRING") && isCaseInsensitive(ast.getChildAt(0))) 
+               {
+                  return true;
+               }

I am so sorry, I was in a rush when I wrote the previous post. Yeah you got exactly right, substring function in 4gl has 4 parameters (source, position, length and type). source and type are of type character, position and length are of type integer. When for snippet from #7604-5 is being executed, !isCaseInsensitive(ast.getChildAt(k)) for position and length return true, causing isCaseInsensitive(substring(source, position, length, type)) to return false, for substring expression.

Also, I had some small irrelevant changes on the trunk, this is why I had to bzr revert to create a patch, and on rewriting changes I had to changes in ExpressionConvertorWorker.java, I made a silly mistake by typing entry instead of substring.

Finally tested with the change on all cases from #7604-2, works as expected. Also works with substring function on case-insensitive.

#11 Updated by Eduard Soltan 12 months ago

Committed on 7464a revision 14664.

#12 Updated by Alexandru Lungu 11 months ago

  • Status changed from WIP to Review
  • % Done changed from 50 to 100

Merged 7464a in trunk as rev. 14673. Archived 7464a.

#13 Updated by Greg Shah 11 months ago

  • Status changed from Review to Test

#14 Updated by Alexandru Lungu 10 months ago

Eduard, now that the change reached customer applications (newer binaries), please check that conversion is OK, so we can close this.

#15 Updated by Eduard Soltan 10 months ago

Yes, conversion seems good.

#16 Updated by Alexandru Lungu 10 months ago

Greg, we can close this.

#17 Updated by Greg Shah 10 months ago

  • Status changed from Test to Closed

Also available in: Atom PDF