Code quality fixes - squid:S1132, squid:S1166, squid:S1854, squid:S1181, squid:S1213, squid:S1488, squid:S2786, squid:ModifiersOrderCheck

[georgekankava]

This pull request is focused on resolving occurrences of Sonar rules squid:S1132 - Strings literals should be placed on the left side when checking for equality. squid:S1166 - Exception handlers should preserve the original exception. squid:S1854 - Dead stores should be removed. squid:S1181 - Throwable and Error should not be caught. squid:S1213 - The members of an interface declaration or class should appear in a pre-defined order. squid:S1488 - Local Variables should not be declared and then immediately returned or thrown. squid:S2786 - Nested "enum"s should not be declared static. squid:ModifiersOrderCheck - Modifiers should be declared in the correct order. This pull request removes technical debt of 155 minutes. You can find more information about the issue here: https://dev.eclipse.org/sonar/rules/show/squid:S1132 https://dev.eclipse.org/sonar/rules/show/squid:S1166 https://dev.eclipse.org/sonar/rules/show/squid:S1854 https://dev.eclipse.org/sonar/rules/show/squid:S1181 https://dev.eclipse.org/sonar/rules/show/squid:S1213 https://dev.eclipse.org/sonar/rules/show/squid:S1488 https://dev.eclipse.org/sonar/rules/show/squid:S2786 https://dev.eclipse.org/sonar/rules/show/squid:ModifiersOrderCheck Please let me know if you have any questions. George Kankava

[gpu]

I have not yet analyzed this in detail, and still have to "match" the changes and the rules. But from reading the rule descriptions and having a short look at the changes:

• https://dev.eclipse.org/sonar/rules/show/squid:S1488 , "Local Variables should not be declared and then immediately returned or thrown" : I think I have to disagree with this one - particularly if this was the reason for introducing the method at https://github.com/DevFactory/JOCL/blob/eceac45101da697188a5d346f971dd3aba27313b/src/main/java/org/jocl/LibUtils.java#L559 - but I still have to re-read what should be achieved with this method
• https://dev.eclipse.org/sonar/rules/show/squid:S1181 , "Throwable and Error should not be caught." : That's plainly wrong. The loadLibrary call does throw an Error, particularly an UnsatisfiedLinkError - and all the contortions in the LibUtils#loadLibrary method are done to cope with this. I agree, this is a very special case (and otherwise, this rule certainly makes sense!). But I'll have to figure out which of the changes are due to this warning, and how the changes interplay with each other, and which ones are actually due to https://dev.eclipse.org/sonar/rules/show/squid:S1166 (which probably makes sense as well)

(The other changes seem reasonable. (When a class has a private constructor, I have the habit of placing it at the bottom of the file, but won't argue about this). I'll have to review it further before pulling it)

[georgekankava]

Sure. Please just let ne know which changes you would like to remove and I will remove them.

[blueberry]

@gpu What happens when you regenerate the code? Are you updating the software you use to generate JOCL to follow these rules too?

[gpu]

The most important thing is to not change Throwable to Exception in https://github.com/gpu/JOCL/pull/9/commits/eceac45101da697188a5d346f971dd3aba27313b#diff-4273e34401c0ed1dbe2ca1da20b710a5L140 (because the Throwable there may be an Error that has to be caught).

What was the reason for introducing the private static OSType getOsType(String osName) method? (It may be OK, as it's private anyhow, but I don't understand the purpose yet...)

[gpu]

@blueberry Good catch, but the changes until now referred to code parts that are not auto-generated (mainly the Pointer and LibUtils class).

I guess that Sonar would also complain about some things in the auto-generated JOCLBlast, and in this case, I would have to change the code generator accordingly. But until now, the checks seem to only have run on JOCL itself. (And only on the Java part. I think that the JNI part could benefit even more from a review....)

[georgekankava]

@gpu Thanks for the comments. PR updated.

[gpu]

I don't want to seem tooo picky, but I'm still curious: What was the reason for introducing the new method https://github.com/DevFactory/JOCL/blob/cf5c0c61a2df2ba5c8d169eedab8c3fe361945b2/src/main/java/org/jocl/LibUtils.java#L561 ? It can only be connected to https://dev.eclipse.org/sonar/rules/show/squid:S1854 or https://dev.eclipse.org/sonar/rules/show/squid:S1488 , but don't really see how or why the new method should be an improvement here...

[georgekankava]

@gpu I will take a look at it tomorrow. Thanks

[georgekankava]

@gpu I think this is this rule. Sorry for forgetting to mention it. We can remove this change if you don't like it. https://dev.eclipse.org/sonar/coding_rules#q=complex|rule_key=squid%3AMethodCyclomaticComplexity|s=createdAt|asc=false

[gpu]

Well, I think it does not bring a benefit (considering that some additional null checks and a method comment might be worthwhile), but I can change this one during the pull (maybe tomorrow, more likely friday).

[gpu]

Thanks for these. I didn't introduce the getOsType method, but e.g. the dead stores and missing stack traces in the log messages are certainly worth being changed.