PUBLIC var declaration issue

[Enrico Maria Giordano]

James,

yes, that's exactly what you get using /a.

EMG

[TimStone]

James,

The problem with legacy code is there is often so much to clean up ! I took a program that evolved from its DOS origins 32 years ago, moved it to windows, and kept adding on "functions". It worked, but I never gave much time to optimizing, or cleaning up variables ... just trapped problems and fixed it.

When I started my "optimization project" 10 months ago, and started developing classes, it certainly cleaned up a lot of the code. However it's hard to maintain 100% concentration on such a massive project.

Ultimately I'll be able to eliminate 100% of any automatic declarations, and have everything super fine tuned ... but it is a massive project. It's one thing to convert the shell of a program, but yet another to maintain / convert all of the supportive business logic !

Until then, I'll just accept the auto declarations ... and be happy when I actually have a .prg compile without any !

Tim

[Enrico Maria Giordano]

Friends,

perhaps you misunderstood the purpose of /a. It just avoids the need of using MEMVAR, that's all. So you still have to declare private and public variables using PRIVATE and PUBLIC keywords and still have to access them using M -> prefix. So using /a you don't lost any of the benefits of variable declaration.

EMG

[James Bott]

Enrico,

So using /a you don't lost any of the benefits of variable declaration.

Maybe I wasn't clear. The problem I have with /a is that publics may be getting used in a function that there is no documentation for at the start of the function. Using MEMVAR at the start of a function provides documentation that those publics are being used. Yes, functionally the program works the same, but for me looking at the code it is different.

Tim,

Similar to your situation, the code I am working on was started in the DOS days and it has only been patched over the years. This code is very large and for me it is very difficult to understand, so the more documentation in the code, the better. Since it is still using PUBLICs and (I think) PRIVATEs, it has the problem that it is very fragile and can easily be broken. And those kinds of bugs are very hard to track down because they are highly dependent on a particular set of circumstances, and thus are hard to repeat.

I dropped all use of PUBLICs and PRIVATEs, I don't know, maybe 20 years ago. Since then I have had way fewer errors, and those that I do get are usually very easy to find.

So, the first step is declaration of variables, and then looking for conflicts. It would be nice to eliminate all the PUBLICs and PRIVATEs, but I realize that is a lot of work. Otherwise, just ensuring there are no conflicts will prevent lots of errors.

James

[Carlos Mora]

Hi James,

There is a warning generated for cDate in the DoSomething() function.

Code: Select all  Expand view

Function Main()
#include "fivewin.ch"

   public cDate
   memvar cDate
   cDate:= dtoc(date())
   
   doSomething()

Return nil

Function doSomething()
   msgInfo( cDate )       // warning generated here
return nil

The MEMVAR cDate is the public declaration. Compiling your sample with Harbour I got 2 warnings:
Compilando TEST.prg...
MENU.prg(14) Error E0004 MEMVAR declaration follows executable statement
MENU.prg(15) Warning W0001 Ambiguous reference 'CDATE'
MENU.prg(22) Warning W0001 Ambiguous reference 'CDATE'
1 Files, 2 Warnings, 1 Errors

what is sth spected: MEMVAR is the declaration, PUBLIC is the runtime (executable) code to create cDate.
PUBLIC and PRIVATE commands get translated to sth like __MV*() functions, I don't remember exactly to what but that's the idea.
Said that, may be MEMVAR should be first, then PUBLIC.
Declaring MEMVAR inside a function gives function scope to the var's declaration, so it should be moved in front of both functions.

Code: Select all  Expand view

Function Main()

   memvar cDate

#include "fivewin.ch"

   public cDate
   cDate:= dtoc(date())
   
   doSomething()

Return nil

Function doSomething()
   msgInfo( cDate )       // warning generated here
return nil

Some time ago I was in the same situation as yours, compiling other's people code, with very 'arguably' coding style, and as PUBLICs and PRIVATEs are not very familiar to me, I had to learn how to deal with them.

May be an approach to a solution can be to create a ch header with all the missing MEMVARs and #include it in every source.

[Biel EA6DD]

Code: Select all  Expand view

#include "fivewin.ch"

memvar cDate

Function Main()
   public cDate
   cDate:= dtoc(date())
   
   doSomething()

Return nil

Function doSomething()
   msgInfo( cDate )       // warning generated here
return nil

[Carlos Mora]

Thanks Biel! I thought it that way ("... Declaring MEMVAR inside a function gives function scope to the var's declaration, so it should be moved in front of both functions.") but then I didn't put it in the code.
Its a good think to have someone paying attention.
Thanks again.

PD: We should organize another meeting like the one in Barcelona, what do you think?

[James Bott]

Carlos,

Thanks for your input. I haven't tried your code yet, but it appears that it won't solve the issue I am trying to solve. Moving the MEMVAR to the top of everything will prevent warnings for undeclared PUBLICs, but that is not what I need.

I am trying to get warnings for every undeclared variable used in every function. This forces me to declare all of them at the top of the function. Note that many of the functions in this app are hundreds or even thousands of lines long. So I need to document all the vars to help me understand the function and hopefully make them more bomb proof. The code is also complicated since many, if not most, of the variable names are seemingly randomly generated strings of letters with no meaning. I presume this was done since when I first worked with this code many years ago, there were no variable declarations, so everything was private. Thus every variable name had to be unique across the entire program.

Maybe I actually could do both. First your way, declaring all the publics at the top of the file, then I would only get warnings for undeclared locals. Once I got all those declared, then I could remove the MEMVAR declarations and get warnings for all the publics. I will write some test code to try this out. Hmm, I haven't even thought about privates yet.

Your thoughts?

James

[Enrico Maria Giordano]

James,

I am trying to get warnings for every undeclared variable used in every function.

This is what I've used for decades: don't use MEMVAR and compile with /a, so that you get a warning for each public or private variable not prefixed with M ->. As an example:

Code: Select all  Expand view

FUNCTION MAIN()

    PUBLIC cVar := "This is a test"

    RETURN NIL


FUNCTION TEST()

    ? cVar // warning here
    ? M -> cVar

    RETURN NIL

EMG

[James Bott]

Enrico,

Well, I see your point and in the long run perhaps it would a better idea, but prefixing every public var use is going require probably tens of thousands of prefixes. And I won't be able to tell if a function is using publics just from looking at the top of the function. Using the MEMVAR statement would only require one change per function and I get the documentation. There are 980 functions in this program so you see the magnitude of this issue.

OK, I could put a comment at the top of each function listing the publics being used (that just came to me).

Will try both methods on some of the code to give them each an evaluation.

Thanks for the input.

James

[Carlos Mora]

I am trying to get warnings for every undeclared variable used in every function. This forces me to declare all of them at the top of the function.

May be it's not so easy. It depends on how vars are being used: a LOCAL can't replace a PRIVATE, because the scope of PRIVATEs go beyond the current function to the called ones. Fortunatelly in your case every var has it's own (weird) name, so it will be easy to find the usage of vars outside the funtion it's been declared and add it to the function parameters.

Maybe I actually could do both. First your way, declaring all the publics at the top of the file, then I would only get warnings for undeclared locals. Once I got all those declared, then I could remove the MEMVAR declarations and get warnings for all the publics.

What's your plan? PUBLIC => STATIC and PRIVATE => LOCAL?
SublimeText and xEdit has wonderfull features to refactorize your code, replacing strings in all files simultaneously.

Anyway, it seems to be a significant task. May be a code analizer can help. There were several for Clipper, althought i've never used them.

[James Bott]

Carlos,

May be it's not so easy. It depends on how vars are being used: a LOCAL can't replace a PRIVATE, because the scope of PRIVATEs go beyond the current function to the called ones. Fortunatelly in your case every var has it's own (weird) name, so it will be easy to find the usage of vars outside the funtion it's been declared and add it to the function parameters.

I understand I can't make PRIVATEs into LOCALs. First I just want to make sure all the LOCALs are declared. Originally none were declared and I don't remember what process I used to find and declare them, so I am not sure they were all declared.

What's your plan? PUBLIC => STATIC and PRIVATE => LOCAL?

My plan is to first declare all the vars, then I'll decide what is next. I am an OOP fan, so my public access vars are all stored in an object. However, for now I will just be glade to get all the vars declared.

SublimeText and xEdit has wonderfull features to refactorize your code, replacing strings in all files simultaneously.

Yes, my editor will do that.

Anyway, it seems to be a significant task. May be a code analyzer can help. There were several for Clipper, although I've never used them.

I have a copy of SNAP and this code was run through that at some point becasue all the function calls made by each function are documented in the code. I will have to run SNAP on a old XP system since I'm sure it is 16bit. If I remember correctly it does do a variable cross reference analysis too. That will be huge!

I also have another code analizer, I think it was called "dAnalyst" but I will really have to dig to find that one. It's probably on a 5.5" disk--it's that old. I do have an old PC with a 5.5" disk I have kept just for that purpose. Of course, it will probably choke on all the new syntax we use in FWH. SNAP may have the same problem.

Thanks for all the ideas.

James