FWD » Core Development » Base Language

Implement the following:

• LIBRARY() built-in function
• MEMBER() built-in function (has conversion to FileSystemOps.member(), but this is currently just a stub)

These just parse a string of the form library<<member>> pulling out the library or member respectively. It is a bit of 4GL silliness since it really doesn't do anything with procedure libraries at all. Though we have no plans to implement procedure library support (which store r-code that doesn't get used in FWD), there is no reason we can't implement these silly parsing functions.

Useful background information on libraries: https://documentation.progress.com/output/ua/OpenEdge_latest/index.html#page/dpabl%2Fmanaging-procedure-libraries.html%23wwID0E53YO

Created uast/library_member.p testcase based off some customer code. However, the use of LIBRARY in the sample doesn't seem to utilize the <<member-name>> syntax. Rather, it just looks for the full path to the library.

Should these be placed nearby SEARCH in FileSystemOps like searchLibrary and searchMember?

Roger Borrello wrote:

Should these be placed nearby SEARCH in FileSystemOps like searchLibrary and searchMember?

It looks like member is already there as FileSystemOps.member. I'll add FileSystemOps.library unless there is protest.

That makes sense.

Ready for review...

Committing to: /home/rfb/secure/code/p2j_repo/p2j/active/4231b/                                                                              
modified rules/convert/builtin_functions.rules
modified rules/gaps/expressions.rules
modified src/com/goldencode/p2j/util/FileSystemOps.java
Committed revision 11479.

uast/library_calls/library_member.p is the testcase. Enter a filename in the first field, then a path with <<whatever>> at the end.

Code Review Task Branch 4231b Revision 11479

This is quite good. Here are some things to fix to make it better.

1. Java regex support is computationally expensive. Performance can be improved by pre-compiling the regular expression and reusing that compiled expression. To do this, make pattern = Pattern.compile("(.*)<<(.*)>>"); into a static final member. By the way, the same compiled regex can be used for both functions.

2. The algorithm for both methods is identical except for the regex group returned, right? This means that a common, private static worker could be used instead of 2 inline duplicated pieces of code. I suggest you implement it as private static character procLibraryParser(character name, int group). Then it is easy to call from both locations.

3. What happens when the input filename is unknown value in the 4GL? I assume that unknown is returned. If so, there should be a quick out for this case.

4. Have you confirmed the following?

• Does it matter whether or not the procedure library actually exists?
• If the proc library exists but the member exists, does it matter?
• Does relative or absolute path names for the library matter?

5. There is a typo librabry in the javadoc.

Made the changes to allow the pattern to be pre-compiled, but it no longer matches the given string.

   private static final Pattern patternLibraryMember = Pattern.compile("(.*)<<(.*)>>");

   private static character procLibraryParser(character name, int group)
   {
      Matcher matcher = patternLibraryMember.matcher(name.toStringMessage());

      return matcher.matches() ? new character(matcher.group(group)) : new character();
   }

   public static character library(character filename)
   {     
      return procLibraryParser(filename, 1);
   }

   public static character member(character filename)
   {
      return procLibraryParser(filename, 2);
   }

I don't see anything obviously wrong. The code seems like it should work.

You will need to add the proper error handling, but otherwise it is fine.

Greg Shah wrote:

I don't see anything obviously wrong. The code seems like it should work.

You will need to add the proper error handling, but otherwise it is fine.

Without changing anything, it started to work. Maybe I didn't notice a typo in my entered data. I'll change the testcase to hardcoded values, and just validate rather than allowing input.

Roger Borrello wrote:

Greg Shah wrote:

You will need to add the proper error handling, but otherwise it is fine.

Without changing anything, it started to work. Maybe I didn't notice a typo in my entered data. I'll change the testcase to hardcoded values, and just validate rather than allowing input.

I had a try-catch on PatternSyntaxException, but that only gets thrown when you are compiling the pattern, so I removed it. The other situations that come up would result in the return of Unknown, so there isn't much else to validate that I can see.

Being more explicit about returning unknown when the input is unknown is a good practice. The fact that the matching will fail and unknown is returned in that same case is an implementation detail that is dependent upon toStringMessage() returning "?". This is a little too hidden for my liking.

Added a simpler testcase uast/library_calls/library_member2.p

Greg Shah wrote:

Being more explicit about returning unknown when the input is unknown is a good practice. The fact that the matching will fail and unknown is returned in that same case is an implementation detail that is dependent upon toStringMessage() returning "?". This is a little too hidden for my liking.

Revisions in 11480 ready for review.

Code Review Task Branch 4231b Reviison 11480

Overall, it is good.

1. In FileSystemOps", move the new @private static method to the proper location in the file as documented in our coding standards.

2. In @FileSystemOps", replace references to "ABL" (which is a trademark) with the more generic "4GL".

3. What about item 4 from #4381-10?

Greg Shah wrote:

4. Have you confirmed the following?

• Does it matter whether or not the procedure library actually exists?
• If the proc library exists but the member exists, does it matter?
• Does relative or absolute path names for the library matter?

None of these matter in the least. It is purely an exercise in parsing. That is why the 4GL documentation indicates: "Typically, you use the LIBRARY function with the SEARCH function to retrieve the name of a library. The SEARCH function returns character strings of the form path-name<<member-name>>. if it finds a file in a library."

It does look like SEARCH needs some work, because when you pass "filename.pl<<member.r>>" the 4GL is able to find the file, and it also searches for the member, assuming that it is a procedure library. Of course in order to go that far, we would have to parse the procedure library. That is a useless exercise if we cannot do anything with the member. But we should be able to find the file portion.

I could easily break it up into the parts, if that syntax is included in the filename, then perform the same as we do now with just the filename portion of the string. Or we could leave that syntax unhandled by our implementation.

It is a useless feature. For now, I see no reason to add it. We could handle the file part, but even that has no meaning in FWD. So for now we consider this a known/planned restriction.

Add the following notes:

• In gaps/expressions.rules, change rt_lvl_full to rt_lvl_full_restr and add a comment at the end of the line <!-- does not support procedure library member syntax -->
• In FileSystemOps.searchPath() javadoc, add a similar explanation (with a bit more detail).

Greg Shah wrote:

It is a useless feature. For now, I see no reason to add it. We could handle the file part, but even that has no meaning in FWD. So for now we consider this a known/planned restriction.

Add the following notes:

• In gaps/expressions.rules, change rt_lvl_full to rt_lvl_full_restr and add a comment at the end of the line <!-- does not support procedure library member syntax -->
• In FileSystemOps.searchPath() javadoc, add a similar explanation (with a bit more detail).

Text in Javadoc:

    * Known restrictions are when the SEARCH function used to search procedure libraries in the 
    * {@link EnvironmentOps#getSearchPath}. This allows 4GL to pass {@code filename} either a 
    * member in a library to search for in the procedure libraries in the PROPATH, or library 
    * member syntax "path-name<<member-name>>". In either case the full path to the procedure
    * library is returned. These are not supported syntax.

Let me know of any updates, so I can close this out.

Minor edits:

    * This method intentionally does not support the search of 4GL procedure libraries (either 
    * in the {@link EnvironmentOps#getSearchPath} or as an explicitly listed library). In the 4GL,
    * SEARCH allows passing either a member to search for in the procedure libraries in the PROPATH,
    * or a specific library plus member using the syntax "path-name<<member-name>>". In either case
    * the 4GL returns the full path to the procedure library while this method does not.  This is a
    * known restriction.

Committing to: /home/rfb/secure/code/p2j_repo/p2j/active/4231b/                                                                                          
modified rules/gaps/expressions.rules
modified src/com/goldencode/p2j/util/FileSystemOps.java
Committed revision 11484. 

private static methods should be after all public static methods in a file.

Greg Shah wrote:

private static methods should be after all public static methods in a file.

Oops. Sorry. Updated.