FWD » Core Development » Base Language

Priority features: GET-KEY-VALUE, PUT-KEY-VALUE, USE, LOAD, UNLOAD

The COLOR and FONT options to PUT-KEY-VALUE do not need to be supported at this time.

For now, we need to add conversion support (for all features mentioned above) and the minimum runtime stubs to make the converted code compile. The stub methods can be placed in EnvironmentOps. The work on the real runtime backing code will be done later.

Greg,

I have followed with the conversion process and found the language keywords are already supported in progress.g and AST, JAST generations. So the *.ast, *.jast files are seems to be correct now. And to implement conversion support we need to implement jast -> java source file generation process.

This is a new implementation task for me and have the questions:
1. I need to write the conversion rules to implement this task?
2. Add stub methods to the EnvironmentOps class to be able to compile conversions?
3. The rule I need to change is: convert/language_statements.rules?

Please let me know if I'm wrong or missed for something.

And to implement conversion support we need to implement jast -> java source file generation process.

No, as far as I know there is no need for work to translate the JAST into Java. The features needed by the conversion should already be there and convert/brew.xml does the source file generation.

1. I need to write the conversion rules to implement this task?

Yes. Your rules need to create the nodes in the JAST tree, which will then emit as source code that calls the backing runtime methods.

2. Add stub methods to the EnvironmentOps class to be able to compile conversions?

Yes.

3. The rule I need to change is: convert/language_statements.rules?

Yes.

Currently there are no significant issues with the statements:
PUT-KEY-VALUE, USE, LOAD, UNLOAD

The small question for these statements is: We need the P2J backend to support both input types, character and String, right. I'm planning to implement the real java code inside for example setKeyValue(String, String, String). The other cases for this call will just use String based "master" like this: setKeyValue(String, character, String)-> transforms character to String by getValue() and calls setKeyValue(String, String, String). Right?

The main issue I'm currently working on is the GET-KEY-VALUE. The Progress uses C-style parameter. This is not a function. like value = GET-KEY-VALUE, this is the statement: GET-KEY-VALUE. The Value is the variable to store the returned value. So we need to transform from the 4GL style: GET-KEY-VALUE to Java style: Value = GET-KEY-VALUE. This is a kind of assignment. I'm going to insert the appropriate modifications into convert/language_statements.rules file leaving annotation assignments rules untouched. Let me know if there are objections.

 We need the P2J backend to support both input types, character and String, right.

Yes.

I'm planning to implement the real java code inside for example setKeyValue(String, String, String). The other cases for this call will just use String based "master" like this: setKeyValue(String, character, String)-> transforms character to String by getValue() and calls setKeyValue(String, String, String). Right?

Good. The only thing here is to put a // TODO implement this comment inside the real worker (setKeyValue(String, String, String)), since we are just stubbing it for now. You can go ahead and write the others to call setKeyValue(String, String, String) since that doesn't take long.

So we need to transform from the 4GL style: GET-KEY-VALUE to Java style: Value = GET-KEY-VALUE. This is a kind of assignment. I'm going to insert the appropriate modifications into convert/language_statements.rules file leaving annotation assignments rules untouched. Let me know if there are objections.

Yes. Generally, in such cases we use a template to help us get the tree structure right. I recommend using "simple_unfinished_assign". It can be done something like this:

            <action>
               lastid = tw.graft("simple_unfinished_assign",
                                 copy,
                                 closestPeerId,
                                 "javaname",
                                 javaname).id
            </action>
            <action>
               createPeerAst(java.static_method_call, "EnvironmentOps.getKeyValue", lastid)
            </action>

You would have to set the javaname variable to the converted name of the variable to assign AND you would have to ensure that it did NOT emit again during normal processing (which it would in variable_references.rules). Most often we hide the variable during annotations processing AFTER we have copied the javaname information into the main statement's (the KW_GET_K_V node).

From there, you can let everything else emit as by default.

Feedback on the testcase:

Please add code that shows the use of:

1. LOAD with BASEKEY "INI"

2. PUT-KEY-VALUE with "?" in section and/or key and/or value.

3. GET-KEY-VALUE with "?" in section and/or key.

I will look at the coding changes next.

Code feedback:

1. The stub methods in EnvironmentOps cannot call character.getValue() (et al) without some unknown protection. Solve this by just wrapping any String/boolean parameters in a wrapper constructor (new character(my_string) or new logical(my_boolean)). The real worker method should be the form that has all wrapper constructors (e.g. getKeyValue(character, character)). That is the place where we will centralize all the unknown value processing before doing any real work.

2. Please remove any extra space at the end of method signatures:

public void myMethod(Type parm1, Type parm2 )
                                           ^

Otherwise, the update looks good.

The real worker method should be the form that has all wrapper constructors (e.g. getKeyValue(character, character)).

You mean the final real worker where we will implement the code logic should be changed from e.g. getKeyValue(String, String) to getKeyValue(character, character)? All possible cases of the getKeyValue() should finally call the getKeyValue(character, character) as worker. Correct?

You mean the final real worker where we will implement the code logic should be changed from e.g. getKeyValue(String, String) to getKeyValue(character, character)? All possible cases of the getKeyValue() should finally call the getKeyValue(character, character) as worker. Correct?

Yes.

The latest testcase is fine. Please check it into bzr (p2j_repo/testcases/uast/).

Another question. In the new approach I need to reformat incomplete calls like this:
getKeyValue(String section)
We have the opportunity to transfer to call either
return getKeyValue(new character(section), new character("?"));
or
return getKeyValue(new character(section), new character());
or
return getKeyValue(new character(section), null);

We need to mark the default behavior for missing parameters. The right way is to set unknown value - new character()? Correct?

We need to mark the default behavior for missing parameters.

Yes.

The right way is to set unknown value - new character()?

I don't know. We can only find this by testing in the 4GL environment. For now, just put TODOs there an do the minimum to satisfy javac.

Please check it into bzr (p2j_repo/testcases/uast/).

Done.

I am OK with the changes. But there is a conflict in bzr. Please merge with the latest changes and upload it.

OK, this looks good. Let's wait until tomorrow for conversion testing, since Constantin is in testing with another change that will conflict with language_statements.rules. You'll merge tomorrow and then do the conversion testing. Runtime regression testing won't be needed.

After reading some background documentation I have got the implementation plan:

1. The "environment" is a set of the variables with defined values organized in (name<->value) mapping pair. The lifecycle of the environment is: load, use, get|put-key-value and unload. Every given time only one current environment can be active. When the Progress starts on Windows it loads the default working environment from registry.

2. I suggest to use separate Java class/source file in com.goldencode.p2j.util.* package, say Environments.java or Registry.java to separate all related handling there including possible support for the multiple environments on demand. We can call the methods from EnvironmentOps to this new class.

3. The code will work under Windows only(correct?). For this the check with existing PlatformHelper.isUnderWindowsFamily() will be performed and if not in Windows - just ignore or exception raising.

4. Looks like we will need to call native code to access the real Windows registry. It is planned to be implemented in Environments.java or Registry.java by JNI to the p2j.dll. Native code to touch the registry is planned to be implemented in registry.c file of the native library source tree and be compiled with MinGW for Windows.

5. The first implementation will work with only default environment reflecting to the registry. Then we add multiple environment handling and INI files.

6. The Java internals will be implemented using HashMap to store key-value pair. When application is finished the environment should be saved back to the storage(if changed).

Is the plan OK? Continue working.

2. I suggest to use separate Java class/source file in com.goldencode.p2j.util.* package, say Environments.java or Registry.java to separate all related handling there including possible support for the multiple environments on demand. We can call the methods from EnvironmentOps to this new class.

It is important to note that the same 4GL features can be backed by either the Windows registry OR by "stanza" INI files. Both need to be supported in P2J.

The INI support can be done in pure Java, but the Windows registry will probably need native code.

Either way, the interfaces on the server that are called from the converted code should be in EnvironmentOps. The low level functions needed should be on the client side, since that is where the INI files or the registry exists. We will need to provide remote access to those functions (similar to how we do it for the FileSystemOps <--> FileSystemDaemon).

We will have to determine exactly the features needed for the registry (which could be in a helper class named Registry.java) and what features are needed for INI support (which could be in a file called StanzaIni.java). If possible (and reasonable), please make the external API for both support classes into an interface called EnvironmentAccessor. Both Registry and StanzaIni should implement EnvironmentAccessor. This way, the EnvironmentOps.load() can create an EnvironmentAccessor instance and save it off, keeping the EnvironmentOps code separate from the code that actually accesses the specific "environment".

Even though the INI file usage can be implemented in pure Java, the file itself will have to be remotely read from/written to the client.

3. The code will work under Windows only(correct?).

I don't know. I suspect that the stanza INI is also supported on Linux/UNIX. Please check.

if not in Windows - just ignore or exception raising.

Please check what the 4GL does and duplicate that behavior.

4. Looks like we will need to call native code to access the real Windows registry.

Yes.

by JNI to the p2j.dll. Native code to touch the registry is planned to be implemented in registry.c file of the native library source tree and be compiled with MinGW for Windows.

Yes.

6. The Java internals will be implemented using HashMap to store key-value pair.

I'm not sure here. For the INI file usage, this may be appropriate (since we don't want to reparse the file for every access). But for the registry, I suspect we need to dynamically call the WIN32 registry APIs.

More importantly, does the 4GL immediately persist any changes to the backing registry/INI file? Or are those changes kept in memory and then saved (or not) depending on future processing?

These questions need answers before we can know the exact implementation.

Although this is a separate 4GL feature, the OS-GETENV is a related functionality. We currently implement this in FileSystemOps.getProperty(). Today, we call into the client (FileSystemDaemon) to query a Java "system property". In the 4GL, this is actually resolved by reading the real environment variables that are part of the client process.

As part of this task, please add native code to back FileSystemOps.getProperty() by first checking the client's environment variables and only if the requested variable is not there, to fall back and use Java system properties. This code can easily be added to p2j.dll and by using common C runtime features, I think it should be very portable. The native method can be added to FileSystemDaemon.

...
As part of this task, please add native code to back FileSystemOps.getProperty() by first checking the client's environment variables and only if the requested variable is not there, to fall back and use Java system properties.
...

OK.

The suggested change for OS-GETENV support:
1. FileSystemDaemon.java:
Declare native method and load p2j:

...
   static
   {
      System.loadLibrary("p2j");
   }

   private native static String getEnvironmentValue(String variable);
...

return System.getProperty(key);

String result = getEnvironmentValue(key);

if (result == null || result.length() == 0)
{
   result = System.getProperty(key);
}

return result;

JNIEXPORT jstring JNICALL 
       Java_com_goldencode_p2j_util_FileSystemDaemon_getEnvironmentValue(JNIEnv  *env,
                                                                         jclass  jc, 
                                                                         jstring variable) 
{
   const char *env_var = (*env)->GetStringUTFChars(env, variable, NULL);
   char * result = NULL;

   if (env_var != NULL && strlen(env_var) > 0)
   {
      result = getenv(env_var);
   }

   if (result != NULL)
   {
      return (*env)->NewStringUTF(env, result);
   }
   else
   {
      return NULL;
   }   
}

The implementation compiled in Linux and Windows (with appropriate change in build.xml) and works fine.

Yes, this looks good.

More importantly, does the 4GL immediately persist any changes to the backing registry/INI file? Or are those changes kept in memory and then saved (or not) depending on future processing?

After some investigations looks like we have clear picture for how it works. According to the Progress ABL documentation (ABL Development reference - dvref.pdf). The "environment" is a unique set of system variable values. Every time in application life cycle it uses "current" set of environment. When application starts it loads the system default environment set which becomes current by default. In Windows it is stored in registry. Then the application can change the current set to the new one by load->use pair taking the new environment either from registry or from INI file. The new current environment becomes active for all frames created after new environment become current by use statement.

The memory occupied by environment remains alive until the unload statement calling. This is the opportunity to free memory or cache the environment for performance if not to call unload.

The testing performed in 4GL on Windows shows the changes are committed immediately. I mean the call to put-key-value() changes the backend either registry or ini file just after returning. There are no buffering or delays before environment changes and the committing this change to registry or ini file. For example simple test:

...
define var chVar as character initial "CharVar0".
put-key-value section "EvlSection" key "EvlCharKey" value chVar. 
<-Here the registry has been changed
message "Press a key to finish.".
pause.
...

...
define var chVar as character initial "CharVar0".
load "evl_env1".
use "evl_env1".
put-key-value section "EvlSection" key "EvlCharKey" value chVar. 
<- Here the ini file evl_env1.ini has been changed
message "Press a key to finish.".
pause.
...

On the other hand we need to support co-existence several inactive environments in memory(ones that loaded but not currently using). So we need to have some in-memory structure to store the section-key-value tree. And I think it should not be dependent on the type of the backend storage(registry or ini file).

Another point is using the default environment modification statement just silently ignoring and does not work. The load statement gives the error message:

The LOAD of evl_env1 in path /home/eugeniel failed. (4449)

The USE of evl_env1 failed. (4452)

I don't understand what you mean here. Please show an example.

And a question: can the INI files be used on Linux/UNIX?

I mean trying to use the code that loads INI file in Linux:

...
define var chVar as character initial "CharVar0".
load "evl_env1".
use "evl_env1".
...

This gives error in 4GL under Linux. I think this means the ini files handling are not supported in Linux because if we can not load INI file - we can not use it. According to Progress docs, these statements work only in Windows so this is expected, I just have not found the explicit note for missing Linux support.

OK. Then you will need to generate the same errors when running on systems other than Windows.

The overall design looks good.

Some thoughts:

1. Please create some non-trivial samples that use a complete range of the features.

2. Make sure these samples include code that exploits the boundary conditions of this feature set. There should be examples of each parameter being unknown value and/or values that are invalid so that the full error processing can be understood. Please see testcases/uast/memptr/memptr_start_get_before_beginning.p for an example of how I have done it recently.

3. Based on this, you should be able to write proper implementations of each method.

4. Don't forget that the ThinClient constructor will need to be updated to create (and save off) a new instance of EnvironmentDaemon. Otherwise the code won't work!

Code Review

1. The use of EnvironmentDaemon.envMap should probably have keys lowercased. The change is needed in the load() method where it is put() and in the use() method where get() is called. I say this because the 4GL is usually case sensitive. Make sure your testcases check this.

2. The approach with the StanzaIni class has 2 potential design issues:

• The algorithm to read the file assumes that there is little to no whitespace in the structure of a given line. For example, a new section is always assumed to have the [ character at the index position 0. What if the 4GL allows whitespace in front? There are other things like this in the current implementation. It will probably need to be refined to make it much more tolerant of structural differences.
• The approach to saving changes completely re-writes the file. This may cause the file's whitespace to be very different from the original (multiple blank lines would be condensed, extra whitespace on lines with content will be eliminated and so forth). Only if the 4GL causes this same behavior, can we use this approach.

Otherwise, the changes look good. Keep going.

Code Review (0618a)

1. Please check if whitespace at the end of strings is ignored for environment names. In the 4GL the string comparison is not only case-insensitive, but whitespace is trimmed from the end of the strings before comparison. This would affect both the EnvironmentDaemon.load() and EnvironmentDaemon.use().

2. The use of String.concat() in StanzaIni.createEmptyIniFile() is broken. concat() returns the modified string it does not modify the contents of the String instance since String is immutable. So the code would have to look more like fileName = fileName.concat(something). Please note that it would be cleaner/faster to use StringBuilder.

3. Does the 4GL always generate a lowercase filename for all newly created stanza INI files? We need to match its case behavior exactly.

4. Both registry.c functions seem to have some common code. You may be able to abstract that code into some helper functions to make the code have less code duplication and be more readable.

Good stuff. Keep going.

LOAD and USE does not really touch the registry for creation keys with given environments.

Can you explain what you mean by this?

LOAD and USE does not really touch the registry for creation keys with given environments.

Can you explain what you mean by this?

The actual registry modification is happening in PUT-KEY-VALUE statement.

The real creation for LOAD env NEW statement is happening only after first PUT-KEY-VALUE statement. In case of using:
...
LOAD env NEW.
USE env.
UNLOAD env.
...
nothing is really created and the registry remains untouched.

1. Please check if whitespace at the end of strings is ignored for environment names. In the 4GL the string comparison is not only case-insensitive, but whitespace is trimmed from the end of the strings before comparison. This would affect both the EnvironmentDaemon.load() and EnvironmentDaemon.use().

OK. I'll check this point.

3. Does the 4GL always generate a lowercase filename for all newly created stanza INI files? We need to match its case behavior exactly.

No, the code

load "EvL_eNV2" new base-key "INI".

4. Both registry.c functions seem to have some common code. You may be able to abstract that code into some helper functions to make the code have less code duplication and be more readable.

OK.

1. Please check if whitespace at the end of strings is ignored for environment names. In the 4GL the string comparison is not only case-insensitive, but whitespace is trimmed from the end of the strings before comparison. This would affect both the EnvironmentDaemon.load() and EnvironmentDaemon.use().

The strange result I've got in testing. The environment names "env" and "env " are absolutely different from the statement
LOAD "env" and LOAD "env " point of view. Meaning:
1. The file env.ini can not be loaded via

LOAD "env  " base-key "INI".

LOAD "env  ".

"env  .ini"(with extra spaces)

"env  "(with extra spaces).

"env  .ini"

"env  "

USE "env  ".

"env "

"env  "

So looks like 4GL in Windows consider whitespaces at the end as meaningfull.

So looks like 4GL in Windows consider whitespaces at the end as meaningfull.

This seems to be true for the LOAD and USE cases only. As far as I understand it, this does not extend to general character type comparisons, which are documented in the 4GL to be insensitive of whitespace at the end of the text.

So looks like 4GL in Windows consider whitespaces at the end as meaningfull.

This seems to be true for the LOAD and USE cases only. As far as I understand it, this does not extend to general character type comparisons, which are documented in the 4GL to be insensitive of whitespace at the end of the text.

Yes, I did mean usage within LOAD and USE statements only.

More INI files interesting findings:
GET_KEY_VALUE for missing entry gives empty string, not error or NULL string.
PUT_KEY_VALUE for value with spaces gives writing of the extra spaces to disk leading and trailing as well, GET/PUT this value again trims extra spaces.
PUT-KEY-VALUE trims any leading and trailing spaces from the key names and write them without errors.
PUT-KEY-VALUE trims any leading and trailing spaces from the section names and write them without errors.

GET-KEY-VALUE trims any extra spaces from key value while reading from INI file if such white spaces in value was in INI file. So writing the same value again by PUT-KEY-VALUE gives truncated value. As an example consider the INI lines:

"EvlCharKey1=  Char Var 1 " 
"EvlCharKey2=  Char Var 1" 
"EvlCharKey3=Char Var 1 " 

get-key-value section "EvlSection" key "EvlCharKey1" value chVar. 
get-key-value section "EvlSection" key "EvlCharKey2" value chVar1. 
get-key-value section "EvlSection" key "EvlCharKey3" value chVar2.
put-key-value section "EvlSectionNew" key "EvlCharKey1" value chVar. 
put-key-value section "EvlSectionNew" key "EvlCharKey2" value chVar1. 
put-key-value section "EvlSectionNew" key "EvlCharKey3" value chVar2.

"EvlCharKey1=Char Var 1" 
"EvlCharKey2=Char Var 1" 
"EvlCharKey3=Char Var 1" 

GET-KEY-VALUE accept the leading/trailing spaces for key and section names, considering them without spaces"
" EvlCharKey 10 " "EvlCharKey 10 " " EvlCharKey 10" referring to the same key name
"Evl Section1 " " Evl Section1" " Evl Section1 " referring to the same section name.

The only exception/rule here is the sections and key name in files before GET-KEY-VALUE must be without extra spaces as it can be after manual modification outside the Progress system. One example is editing INI file with Notepad. Only key value can have the leading/trailing spaces in INI file.

The code in 0621a is good. The findings are also interesting. Please make sure to test cases where "" (empty string) is passed for various parameters AND where unknown value is passed for various parameters. Generally, each test should only pass "" or unknown in one parameter and then normal input for the other parameters. Also note that you will probably have to pass unknown value like this:

def var the-unknown-var as char init ?.
get-key-value section the-unknown-var key "some-key" value chVar

This is often needed because the 4GL compiler will usually disallow the direct use of the unknown value literal ?.

Good stuff. Keep going.

Code Review 0626a

I am fine with the code and approach in general. Please note that the StanzaIni constructors need javadoc. I also do prefer for the EnvironmentAccessor interface (and thus also the EnvironmentDaemon, Registry and StanzaIni classes) be implemented using String instead of character. If there is some behavior that requires character to be used instead, then continue using it. Otherwise, do shift to String. That should also mean that the "unwrapping" and unknown processing can be done in 1 place (EnvironmentOps) which is cleaner.

How much more effort do you expect is needed to complete your testcases and the implementation?

How much more effort do you expect is needed to complete your testcases and the implementation?

This is the serious question. I'm trying to work in parallel on INI and registry sections(to be able to think on background task while implementing another) to complete this ASAP. But I've found we need to cover many possible situations, many options, create testcases to see what is happening on real 4GL system and more important to understand why it is happening. So I think at least 2 weeks more to complete is more or less real estimate. Sorry if this is too long. The environments is the point when the error can potentially damage the production system destroying the INI files or registry and I would like to do my best in this task.

The code looks good. Some thoughts:

1. Please do consider if it is possible to shift the EnvironmentAccessor to use String instead of character.

2. Does the unrecognized line processing also handle comments?

3. Does the unrecognized line processing even handle comments/whitespace at the beginning of the file (which is not part of any section)?

4. Can there be whitespace or comments in the middle of a section (with real/working content following the whitespace/comments)?

5. I think it may make sense to keep the comments/whitespace separate from the map of maps of key/value. The current approach is good for key/value usage but it is awkward for comments and whitespace. I would prefer you to add some classes:

Content (abstract base class)
LineContent (child of Content which contains a string with any whitespace/comments)
SectionContent (child of Content which contains the section name and a reference to the given section's LinkedHashMap of key/value content)

Then you can maintain a LinkedList<Content> which will handle all the ordering issues and can keep the LineContent separate from the SectionContent.

This approach would need some modification if there can be whitespace/comments in the middle of the section content... Anyway, this may give you some implementation ideas to make a cleaner (and more modular) approach.

Please implement this exactly like the 4GL actually works. It doesn't matter what the documentation says. We have to make real applications work 100% compatibly with the original. That means we implement all the undocumented and crazy behavior.

Code Review 0701a

1. I would like some changes made to the Content class and its subsclasses.

• Do not store "name" in the Content class. This variable only makes sense for a key/value pair and is not a good description for line text.
• You should not need the "withName()" method (see below).
• Please rename the method writeToFile() to write().
• LineContent can store its own data as an instance member called "text".

2. The algorithm for StanzaIni.getContent() is more complicated and slower than needed. Please add a HashMap<String, Content> instance member to StanzaIni. You would add SectionContent instances to that map when they are created and you never have to add LineContent instances to it. Then getContent() becomes a simple map lookup and you no longer need Content.withName().

3. SectionContent should store its own key/value data in a map BUT I really don't want to store the comments and empty lines in the same map. It is messy and confusing. I think it is better to use some simple inner classes and a new list:

List<Renderable> ordering = new LinkedList<Renderable>;

public interface Renderable
{
   public String render();
}

private static class TextRenderer
implements Renderable
{
   private String text = null;

   private TextRenderer(String text)
   {
      this.text = text;
   }

   public String render()
   {
      return text;
   }
}

private static class KeyRenderer
implements Renderable
{
   private String key = null;

   private Map<String, String> lookup = null;

   private KeyRenderer(Map<String, String> lookup, String key)
   {
      this.lookup = lookup;
      this.key    = key;
   }

   public String render()
   {
      // code to lookup the value from the key/value map and then output it as a formatted string ("key = value")
   }
}

This would easily allow the key/value stuff to be inserted in the list as KeyRenderer instances and all whitespace/comments to be added as TextRenderer instances. The whitespace/comments would no longer be kept in the section key/value map. It should make output easy too.

4. What happens if the same section name appears more than once (2, 3 or more times) in the same stanza INI file?

2. The algorithm for StanzaIni.getContent() is more complicated and slower than needed. Please add a HashMap<String, Content> instance member to StanzaIni. You would add SectionContent instances to that map when they are created and you never have to add LineContent instances to it. Then getContent() becomes a simple map lookup and you no longer need Content.withName().

This means we have to support two collections in StanzaIni class:
1. One is LinkedList<Content> to keep ordering of the lines in the file
2. Another one is HashMap<String, Content> to handle name->section mapping.
Both of them will have the same <Content> entries. Is my understanding correct?

Yes.

Additional findings for same section name in one file. Even we have the following:

...
[Section1]
key1=value1
[Section1]
key2=value2
...

Code Review 0702a

The code looks good. Some comments:

1. Please put a TODO into StanzaIni to implement the PrintWriter creation the using proper charset (not always just with the default for the JVM).

2. Have you checked to see what happens if the same key appears more than once in a single section?

1. Please put a TODO into StanzaIni to implement the PrintWriter creation the using proper charset (not always just with the default for the JVM).

OK.

2. Have you checked to see what happens if the same key appears more than once in a single section?

In this case only first occurrence line with the duplicated key has meaning. The other lines are silently ignored.

2. Have you checked to see what happens if the same key appears more than once in a single section?

In this case only first occurrence line with the duplicated key has meaning. The other lines are silently ignored.

I think that loadIniFile() will need changes to process this properly.

You may need to check if the duplicate keys are deleted on output or if they are just silently left behind.

I'm planning to handle this case as set of the ignored lines not belonging to any sections like comment in the head of the file if you do not mind.

OK.

Another interesting finding. Consider the situation when there are two keys with same name and different values in one section:

...
CharKey2=CharVar2
CharKey2=CharVar3
...

Code Review 0708b

I am fine with the changes. One thing I see is that Javadoc which states If the parameter is null string("") the is confusing since a String instance can be EITHER null OR the empty string "". Please change that text to state empty string ("").

We need to get this task completed. What is your current estimate of the remaining time?

Code Review 0710a

1. The Registry.java class javadoc needs significant enhancements. The native methods have only a 1-line description. It does not really tell anything about the full details of how those methods work. Even the non-native methods need much more documentation. Every parameter needs to be completely documented, especially since most of the complexity of the behavior comes from different combinations of parameter edge-cases.

2. As a general rule, all of the files are lacking complete javadoc about what happens when a parameter is null or unknown value. Some of the parameters are missing documentation on what happens when the parameter is an empty string.

3. There are many places in the code that are structured like this:

      if ((section == null || section.isEmpty()) && (key != null && !key.isEmpty()))
      {
         ...
      }
      else if ((section == null || section.isEmpty()) && (key == null || key.isEmpty()))
      {
         ...
      }
      else if ((section != null && !section.isEmpty()) && (key != null && key.isEmpty()))
      {
         ...
      }
      else if ((section != null && !section.isEmpty()) && key == null)
      {
         ...
      }
      else
      {
         ...
      }

This structure is very hard to read (especially when you add the hidden code back in. The duplication of condition processing makes the reader have to carefully evaluate what is going on and the conditions are so complex that it takes much longer than it should to do that analysis. This will make the code hard to maintain and fragile to regressions and bugs. Please look through this update for such cases and replace them with an approach like this:

      if (section == null || section.isEmpty())
      {
         if (key != null && !key.isEmpty())
         {
            ...
         }
         else
         {
            ...
         }
      }
      else
      {
         if (key == null)
         {
            ...
         }
         else if (key.isEmpty())
         {
            ...
         }
         else
         {
            ...
         }
      }

Please take this approach in all of your code in the future. Let me know if the reason for this unclear.

Finding news for handling registry in Windows 4GL in PUT/GET KEY-VALUE calls.
1. The CHUI and GUI interfaces works the same.
2. The "" and unknown-var considered the same way.
3. The crazy news has too. For both interface types the behavior differs from what is described in Progress documentation:
The case when section "" and key "" GET-KEY-VALUE should return the comma separated section - key pairs in the form:
"section1@key1,section1@key2,...,sectonN@keyN"
But actually this case returns just comma separated list of the sections in format "section1,section2,...,sectionN".

So I need to make the appropriate changes in native code to take into account #3.

OK.

Code Review 0712c

1. Do LOAD/UNLOAD/USE really generate a STOP condition (like a CTRL-C) on non-Windows?

2. Normally, when NO-ERROR is active the ERROR-STATUS handle can be read to get the error message, error number and so forth. This is handled in ErrorManager using the RemoteErrorData.addRecord() method. But showErrorAndAbend() does not implement this feature. Please check if you can see the error data using ERROR-STATUS after you "catch" the STOP condition using an extra containing block like:

REPEAT ON STOP UNDO, LEAVE:
  /* code that uses LOAD/UNLOAD/USE on non-Windows platform */

  LEAVE.
END.

/* code that checks ERROR-STATUS */

3. Why are there comments like // TODO implement this in these files? Are these really still open TODOs?

4. Are all your testcases working exactly like in the 4GL?

1. Do LOAD/UNLOAD/USE really generate a STOP condition (like a CTRL-C) on non-Windows?

Yes, when error is enabled these statements restarts the application. If no-error - silently ignoring.

2. Normally, when NO-ERROR is active the ERROR-STATUS handle can be read to get the error message, error number and so forth....

OK. I'll check this.

3. Why are there comments like // TODO implement this in these files? Are these really still open TODOs?

I've left them for possible changes reflecting to your notes. Do I need to remove them?

Note the PrinterWriter TODO is actual because there is only default charset is now works.

4. Are all your testcases working exactly like in the 4GL?

Yes, I've tested single functionality with single test. Then I debugged the code to fix deviations. So the the goal was to have identical results on Windows in 4GL and P2J. And I see no difference for test behavior for now.

I've left them for possible changes reflecting to your notes. Do I need to remove them?

If your testing proves that the implementation is correct and complete, then please remove the TODOs. If something is still missing, then put a more specific TODO in place to make it clear.

Note the PrinterWriter TODO is actual because there is only default charset is now works.

Yes, good.

4. Are all your testcases working exactly like in the 4GL?

Yes, I've tested single functionality with single test. Then I debugged the code to fix deviations. So the the goal was to have identical results on Windows in 4GL and P2J. And I see no difference for test behavior for now.

When you say you tested a "single test", do you mean that you ran each of your testcases individually? In other words, it is important that ALL of your testcases are working, not just one of them.

If your testing proves that the implementation is correct and complete, then please remove the TODOs. If something is still missing, then put a more specific TODO in place to make it clear.

OK.

When you say you tested a "single test", do you mean that you ran each of your testcases individually? In other words, it is important that ALL of your testcases are working, not just one of them.

To be exact I should say "every single test" instead. Yes I ran each of the test individually. From first to last. Debug, fix the code and create new test and so on. So ALL testcases are working.

Code Review 0717a

Each code review I look deeper in some areas and catch some things I didn't see before. We are getting close, but there is some cleanup to do.

1. The EnvironmentAccessor interface is designed well for use as the remote interface to EnvironmentDaemon. But it is not a good fit for the "low level" Register and StanzaIni classes. In particular:

• The use() method is not needed in either the Registry or StanzaIni classes.
• The load() method arguments are not used in either the Registry or StanzaIni classes.
• The unload() method arguments are not used in either the Registry or StanzaIni classes.

Please create an EnvironmentReader interface, that is a simpler/minimized version of EnvironmentAccessor. Change the Registry or StanzaIni classes to implement EnvironmentReader instead of EnvironmentAccessor.

2. In Registry.java, the javadoc has "defailt" instead of "default".

3. There is dead code in Environments.WorkArea: remove the lastError comments.

4. In regard to the error/abend processing:

The testcase env_test34.p shows error-status:error == no on 4GL Linux system. The current EnvironmentOps code also has the same behaviour with usage showErrorAndAbend(). The only issue here is the showErrorAndAbend() has additional internal LogicalTerminal.pause() integrated which 4GL does not do. Can we make the method similar to showErrorAndAbend() but without internal LogicalTerminal.pause()? Because looks like 4GL does not consider INI/Registry code as deserved to produce the real error if encountered in Linux. The application just stops when NO-ERROR is not defined.

Yes, it makes sense to have a new method in ErrorManager that provides the proper behavior. Please name it conditionalShowErrorAndAbend() and make sure that it handles the NO-ERROR mode too (it should return silently if we are in silent error mode). That way you can further simplify the calling code to avoid the call to ErrorManager.isSilentError().

Code Review 0718a

It looks good. Get it runtime regression tested on devsrv01.

Finally the update has passed all regression testing in devsrv01 except known TC-JOB-002(including CTRL-C 3-way tests). I'm going to merge it with the recent P2J codebase and upload here.

I think the merged changes are safe enough. Go ahead and check it in (and distribute it via email). Good work!

Committed in bzr as 10367. Will distribute soon.