#53 Fix #777 Undeclare in c file

[sbelondr]

Hello everyone,
Sorry, I don't know how to send a pull request with svn. So, I send it to you with a ticket.
I think I have fixed ticket number 777. For that I've edited field.c and codegen.c files.

Thank you.

[Simon Sobisch]

Thank you for the note and for taking the time to share your patch. Are you sure it reference [bugs:#777]?

In an case please include a ChangeLog entry in patches as this helps a lot to understand changes without getting the complete context of the change before.

The second part (field.c) looks both unrelated, not sure if it correct.
It checks if any given field's "top parent" has a redefine and if it has sets that fields global attribute to global if the "any" field has a global attribute. Despite having no clue how this relates to bug 777:

• a GLOBAL attribute may only be set at level 1 (but is internally inherited down, so it would be reasonable to first check f->flag_global, then doing the check)
• technical-wise you likely would want to use cb_field_founder to get the top parent
• the standard is explicit that when a REDEFINES entry has GLOBAL that it only applies to this item, so I'm not sure if it implies the contrary, but I don't think so
• rechecked with MF (ma be wrong, too): it doesn't matter for a data-name that has aREDEFINESif its "target" has GLOBAL or not. To be found in a nested program it needs to have the GLOBAL clause itself.

For the first part (codegen.c): can you please provide a testcase for what this is actually fixing? Ideally in the patch as testsuite entry under tests/testsuite.src, but attached to this tracker would be ok, too,

[sbelondr]

In the archive you can find the patch with the Changelog and the testsuite.

Second part:

In file included from main_2.c:21:
main_2.c.h:23:34: error: ‘b_8’ undeclared here (not in a function)
   23 | static cob_field f_10   = {3000, b_8, &a_1};    /* L1 */

In the header file, when program call b_8 variable, it's reference to L (parent) not L1 (redefine).
I thought it was needed to declare the parent in global as the redefine, because L1 is never call in output_base (it use parent with real_field_cache function).

[sbelondr]

You can find the fix and the test case in the patch file.

The first problem was the output_index function. This function sets have_sign to 1 before going into output_integer function. And when the program goes in the output_non_local_field_cache, this variable is not found because have_sign is set to 0.

The second problem comes from GLOBAL REDEFINES because the REDEFINES is not checked if it's global in the output_base function.

[sbelondr]

Sorry @sf-mensch, I ping you again. Do you have any information if this patch is right for you ?

[Simon Sobisch]

The patch itself looks fine now, haven't checked the details but it looks small enough to do so soon, likely when pushing the other one.

There's one minor thing in the patch - only related to the ChangeLog entry and GC-specific (the patch doesn't need to be redone):

• there is an empty line after the date/name (currently missing)
• we normally only add to tests/ChangeLog when doing restructurings or global changes

As I've not answered the side question of this issue: sending a patch via issue tracker (preferred as this makes the discussion easier) or via mail is the correct approach for "kind of PR" with svn. Thanks for taking the time to do it well.

[sbelondr]

Hello, thank you for your answers. I have two last questions:
Do you have a rough idea when the patch will be released?
Will it be available for version 3.1 or 3.2 ?
Thank you for your time .

[Simon Sobisch]

There won't be any 3.1 release, 3.2 is overdue, the RC is planned for this month (including the change).

[Simon Sobisch]

It seems both issues are solved in 3.2 dev (both samples compile with GCC on x86_64). @sbelondr can you confirm this and recheck the samples from [bugs:#777] please?

Note: depending on the commits that likely fixed that, we may should still add the tests you've provided.

[sbelondr]

Yes, I tested without this patch and I confirm that it works (3.2-dev).

[Nicolas Berthier]

@sf-mensch This may be closed now. Or are there some further checks to be done w.r.t this?

[Simon Sobisch]

The only things pending are:

• reference commit(s) that fixed this (would help if one has to patch a local install)
• check via this reference if we have a testcase that covers this issue - otherwise the provided testcases should be added - quickest check is possibly: just add them, then inspect code-coverage before and after, if it isn't increased at all we don't need a testcase to be added

Would you mind having a look at this?

[Nicolas Berthier]

Thanks for the tip! Adding the tests do not change coverage… trying to find the "fixing" commit, I could not find any so the tests do not appear to trigger the original bug. Yet I could reproduce the error with [r4872] using the attachment in [bugs:#777] (which I've re-opened). At last, the patch above does not fix the issue.