Riverside-Software / sonar-openedge

CABL (Code Analyzer for ABL in SonarQube) - ABL ANTLR4 Parser
https://riverside-software.fr
GNU Lesser General Public License v3.0
63 stars 26 forks source link

FP on NO-UNDO not specified in temp-table declaration #365

Closed stefandrissen closed 5 years ago

stefandrissen commented 7 years ago

When a temp-table is defined LIKE a NO-UNDO temp-table it is also NO-UNDO.

define temp-table ttone no-undo field cc as char.

define temp-table tttwo like ttone.

define temp-table ttthree undo like ttone.

message 
   temp-table tttwo:handle:undo skip
   temp-table ttthree:handle:undo    
view-as alert-box.
gquerret commented 7 years ago

Seems right, will be fixed.

gquerret commented 7 years ago

A quick glance at the implementation of the rule shows that it's explicitely required to specify NO-UNDO (even if inherited). This behavior comes from ProLint, I'm sure @jurjendijkstra will have an opinion. My opinion is that it shouldn't be required, but there could be an option to require.

jurjendijkstra commented 7 years ago

welll.... a long long time ago I had the opinon that source-code better be specific and not rely on defaults.

Since the default for almost everything is that things are UNDO, and your human coworker sees a declaration without explicit NO-UNDO, then he might quickly think that it is UNDO.

Now in this case the temp-table that has no UNDO and no NO-UNDO is actually surprisingly NO-UNDO, but the collegue has lost some time figuring that out since he is not as quick as a compiler, and specifically specifying NO-UNDO does not take so much time.

So my opinion was to be specific to not confuse your coworkers and yourself. Actually I think I still have that preference.

Isn't the definition of "code smell" not a statement/syntax which is not actually wrong but confusing for others, bearing the risk that they either spoil time or create bugs based on their confusion?

gquerret commented 7 years ago

Having it as an option (e.g. enforce NO-UNDO even when inherited from parent variable / TT / ...) would be the best idea.

gquerret commented 7 years ago

And +1 for your definition of code smell !

stefandrissen commented 7 years ago

A temp-table that is defined like another inherits that ones undo status. That parent temp-table will already have flagged an undo warning (unless overridden).

Relying on defaults is part of knowing your language. I just cringe when I see statements like:

DEFINE VARIABLE lok AS LOGICAL NO-UNDO INITIAL FALSE.

If I were to exaggerate, then I could say this should also be:

DEFINE VARIABLE lok AS LOGICAL NO-UNDO INITIAL FALSE FORMAT "yes/no" COLUMN-LABEL "lok" LABEL "lok" HELP-TEXT ? VALIDATE-TEXT ? etc

Adding all these defaults only, imho, adds noise to the code so that you miss the tree in the woods.

But an option is fine (as long as it defaults to sensible behavior ;-))

jurjendijkstra commented 7 years ago

Yes, opinions and preferences are different and that's cool, I am certainly not a style freak. Sometimes preferences have an history (which is not always an excuse) . Personally I got to love adding "INITIAL FALSE" to the define var as logical because of problems I had experienced in some old C++ compiler. It turned out that while I was programming c++ and trying the code it would use a "debug" build that actually initializes all variables to null, which resolves to false, but the "release" build did not! So customers would have bugs that I could not reproduce with debug. So... I started to get used of adding the initial value and found it the natural thing to do, even in 4GL.

Similarly, I also like to add brackets around logical expressions (involving NOT, AND, OR) so I don't have to rely on my understanding of execution priorities. In other words I do not want to minimize code, I just want to make it clear in code what my intentions are, rather with keywords than with comments.

gquerret commented 5 years ago

Option added in develop branch