Configurable uninproc
I run into the problem that rule uninproc tagged a lot of procedures in "library"-super-procedures.
It turned out the super-procedure had some "public" procedures and they where calling "private" procedures.
But since uninproc can not detect the usage of the "public" procedures it tagged them as not used an that caused the "private" to be also tagged as not used.
I thought this was kind of overkill so I created a configurable property TagDeadParents for rule uninproc which will do (as described in the properties.p):
if TRUE, procedures only called by other procedures, which themselves are never called (DeadParents), cause a warning.
if FALSE, procedures which are only called by other procedures, which themselves are never called (DeadParents), will still be considered used and will not cause a warning.
Files to accomplish this are attached. Place the uninproc.p in the custom/rules folder and merge the properties.p with custom/prolint.properties.p.
To Jurjen:
How would I share these 3 files through CVS?
- Login to post comments
New uninproc
After discussion, the new non-configurable uninproc without the false positives.
Completely rewritten, so please test.
Thanks, but it can be made a
Thanks, but it can be made a lot more efficient now.
It does not matter anymore if the call to internal procedure X has a stack that begins in the main block. That means that as soon as you find a "RUN x" statement anywhere, you can immediately remove X from the list of suspicious procnames.
This means you can do with exactly 1 "RUN SearchNode", no need to set pragma's to remember you have inspected the statement before, no need to populate the tt_ProcedureTree temp-table, and no need to loop through that temp-table to determine if calls have a stack that begins in the main block.
In fact, since the rule is much simpler now, one could argue that the rule can be made somewhat more precise:
- an ip that is marked as PRIVATE must be called from within this c.u., else you are sure it is obsolete
- an ip that is specifically marked PUBLIC is *probably* called from an other compilation unit, so I would not raise a warning about these
- ip's that are not marked either PRIVATE or PUBLIC and are not called from within this c.u. are suspicious, but are maybe called from an other c.u.
More efficient...have you looked at the code ;-)
I think I did exactly what you suggest, the code indeed now has only 1 "RUN Searchnode".
But haven't implemented the PUBLIC/PRIVATE part.
Of course I have looked at
Of course I have looked at the code. It runs n+1 times SearchNode, where n is the number of internal procedures. Unless there is something wrong with the attachment itself.
for each tt_procedure
where tt_procedure.proctype = "PROCEDURE" :
/* Find where procedures are run. */
run SearchNode (
input tt_procedure.startnode,
input "BuildList":U,
input "RUN":U ).
end.
OK, sorry, I'm quit new at
OK, sorry, I'm quit new at this, but won't your code miss the runs from within the "main program" (hTopNode)?
Or does hTopNode have an entry in tt_procedures (haven't looked yet)?
BTW I HAVE replaced the attachment with a version which runs SearchNode 1 time (not from within any loop, so not n+1).
Arrrrgh this bloody
Arrrrgh this bloody web-browser downloaded the previous version of the attachment, instead of the new one. Stupid browser downloaded from its own cache :-(
I have the new version now and confirm it has one single SearchNode. Sorry about the confusion. I will do a closer review later this evening.
Correct: hTopNode is the root of the node-tree from Proparse. hTopNode contains everything, also the "main program". The main program does not have a record in tt_procedures.
Thanks again!
scope
Can/should we perhaps add the scope PRIVATE/PROTECTED/PUBLIC to the tt_procedure temp-table (and perhaps linenumber and sourcefile while your at it)?
With the new OO-code probably more rules will need this information.
re: scope
Yes we can :-)
The scope "private"/"protected"/"public"/"" is probably a good idea, if it is data that is used by more than 1 rule. Until it is re-used I would rather have a function string=GetScope(Node) in hLintSuper.
I would not store linenumber and sourcefilename in the temptable, because they are easy to fetch the moment you need them with 2 simple functions.
Check in
See you have been busy modifying and creating rules.
Are you going to check in this version of uninproc as well?
Niek
Or in a different rulename.
Or in a different rulename. Yes, sure I will check this rule in. But I was wondering.. rules are not configurable but instead it is configurable which rules you want to use or ignore. So if people want the original behavior, and we dont want the rule itself to be configurable, then we should probably rename this new version so people have both and can choose which one to disable.
keep the original
So now you want to keep the original behaviour with what we decided were false positives?
Then I think the original should be renamed and/or moved to contribs.
Interesting idea. But people
Interesting idea. But people who already configured that "uninproc" should be skipped because of the false positives, will after the proposed name-swap not be running your improvement but will be running the old version of the rule instead.
It's still a nice idea to move the original "uninproc" to prolint/contribs/rules but not very practical because it won't run until the setup.exe deletes the version from prolint/rules.
Hmmm.
Hmmm, so people should read the release notes ;-) so they know there is a new improved uninproc?
No, OK, use/check-in as you like.
Sounds like just the right
Sounds like just the right set of rules to me.
So, could you summarize the
So, could you summarize the rules that you think this implements? Hard to review if one isn't sure of a consensus on the spec!
Never mind, the new spec is
Never mind, the new spec is already clear from the discussion, I had no problem reviewing it.
Re: configurable uninproc
Hi Niek,
If a (private) internal procedure is called from a public internal procedure, then that (private) procedure should not be tagged as "not used". Regardless if the public procedure is or isn't called from within the scope of the compiler. If uninproc did raise a warning for those private ip's then I would say that those were false positives.
I think you have improved the rule by solving those false positives. I do not want a configurable option to get that false positive back.
In general I don't like rules to be configurable through prolint.properties. Yes I know there are some in there already, but I don't like it.
Actually I also don't like the name 'DeadParents', the name gives unneeded associations with a family drama.
So what I would submit into SVN is a new version of uninproc.p where that false positive is solved without the use of any DeadParents property.
Intended behaviour vs false positives
I don't agree.
In the comments of the rule it states:
What we need to do now is traverse the tree and set all Children of used procedures to Used.
This indicates that the behaviour is deliberate.
And I think it is a way of looking at it.
If I where to remove the UnUsedParent (or shall we make a "dead parrot joke"?) and Lint the file again I WOULD get warnings on these children. So these "false positives" inform me that if I remove the UnUsedParent I should also remove these UnUsedChildren.
Re: intended behaviour vs false positives
Lets see if I get this right. An example file has two internal procedurs: A and B. procedure B is called from procedure A. Procedure A does not seem to be called at all.
The original code of uninproc gives two warnings: internal procedure A is not used and internal procedure B is not used.
You find that overkill and suppress the warning about procedure B, so the rule only warns that procedure A is not used.
I agreed with you, saying that the warning about procedure B was a false positive.
But now you don't agree with me and defend the warning about procedure B.
So... issue closed?
The discussion with Jurjen
The discussion with Jurjen is about the switch, intended behaviour and false positives.
When I read the original code it seems to me the "false positives" are intended behaviour and so are/were not "false positives".
I on the other hand don't want them so I adjust the code.
Because I don't want to delete the intended behaviour, because I can also see people wanting this, I build it with a switch.
Perhaps we should create a Poll as to what the behaviour of the rule should be ;-)
But perhaps in version 74 we see that the behaviour of uninproc has changed ;-) (because Jurjen found some time to change it)
When I have some time I will rewrite uninproc to only warn about procedure A.
Maybe I'm missing something
Maybe I'm missing something here, but why is there ever a warning about B if it is referenced by A. Seems to me the only thing that should be questioned here is A since it is not referenced by anything internal, but rather is an entry point when this procedure is used as a PP or SP. Right?
Unless one has some signal to indicate that the procedure is used as a PP or SP, I don't see how one can not warn about A since otherwise the whole idea of warning about unreferenced procedures would be meaningless. Only by explicitly flagging it should it be omitted.
If the problem here is that B is being reported because it is referenced only by something that is not referenced, then it seems to me that is the problem. It *is* referenced and therefore should not throw a warning.
The only putative "error" here is A. And that one has to report unless it is flagged or one couldn't report anything. If, in fact, it is a real error that A is not referenced, then one of two things happens. Either one references it, in which case all the things it references are now reached, one one eliminates it, and *then* all the things it references would show as unreferenced (unless they are referenced elsewhere).