-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply CC fixes to existing code #700
base: dev
Are you sure you want to change the base?
Conversation
@@ -188,19 +188,17 @@ private IASTNode getMergedNode(final CLocation otherCloc) { | |||
return otherNode; | |||
} else if (otherNode == null) { | |||
return myNode; | |||
} else // we have two nodes and want to merge them; if one of both is a translation unit, we take the other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging else and if is a good idea in general, but it looks somehow weird, if there is a comment in between, but I don't know how to handle this.
"You selected the library mode (i.e., each procedure can be starting procedure and global " | ||
+ "variables are not initialized). This program contains a \"main\" procedure. Maybe you " | ||
+ "wanted to select the \"main\" procedure as starting procedure."; | ||
final String msg = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really a nice usage of multiline string, we were able to get rid of the quotes \"
.
result = prime * result + (mIsRestrict ? 1231 : 1237); | ||
result = prime * result + (mIsVolatile ? 1231 : 1237); | ||
return result; | ||
return Objects.hash(mIsConst, mIsExtern, mIsInline, mIsRestrict, mIsVolatile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's so much nicer, Boolean::hashCode
returns exactly 1231
or 1237
and Objects::hash
computes exactly this combination.
...ltimate/plugins/generator/cacsl2boogietranslator/preferences/CACSLPreferenceInitializer.java
Outdated
Show resolved
Hide resolved
...informatik/ultimate/plugins/generator/cacsl2boogietranslator/CACSL2BoogieBacktranslator.java
Outdated
Show resolved
Hide resolved
...i_freiburg/informatik/ultimate/cdt/translation/implementation/base/FunctionTableBuilder.java
Show resolved
Hide resolved
...e/uni_freiburg/informatik/ultimate/plugins/generator/cacsl2boogietranslator/Boogie2ACSL.java
Outdated
Show resolved
Hide resolved
The current rules remove variables that are seemingly unnecessary as for example in the following example.
However these variables are not unnecessary they
|
Such measures tend to indicate an unfavorable naming of a function or its excessive complexity. In this case, one should rather give the function a meaningful name or reduce its complexity. To debug the return value of a function, you can use watch expressions in the Eclipse debugger. Watch expressions are also useful for expressions like |
Sounds plausible but
|
Newer Eclipse versions also display the return value of a called function without a watch expression. The only downside is that you see it only when returning from the function, i.e., not anymore in the next step (as far as I know).
In Ultimate, you are probably always able to rename the methods you are calling. I think in many circumstances it is helpful and increases readability if you use appropriately named local variables instead of inlining expressions. If the return of your method is itself a method call, however, then the description of that value should already be clear from the method you are in (not the one you are calling), because this is now the value you are returning. If it is not clear from the caller name, the callers docs, the callees name or the callees docs, it is probably more appropriate to add a line comment there, as this value is not some intermediate result but the result of the function. |
The question is not only what is doable, but what is efficiently doable. Debugging is time-consuming, we have to make sure that developers can debug efficiently. (E.g., maybe you want to set a conditional breakpoint that puts the return value of the method in relation to other values) |
Yes, it seems that Eclipse does not support referencing return values in breakpoint conditions. |
628260f
to
f05c904
Compare
After yesterday's discussion, I changed the coding conventions to make them slightly less strict (see 4661161; incl. variables before returns) and re-applied the coding conventions to |
f05c904
to
e0f5ee4
Compare
During the update to Java 21 (see #672), we also slightly changed our coding conventions.
This can lead to unnecessary diffs, when editing existing files (many existing file did not even follow our old coding conventions...).
In this PR I applied the new coding conventions automatically (starting with
CACSL2BoogieTranslator
) and we can discuss possible changes to the coding conventions here.Eclipse does not actually allow to apply the save actions automatically to multiple files. But I found a way 😉
Clean Up
that can be applied on a whole project under theSource
menu (Note: There is also a commandFormat
there, but it only applies the formatter).Clean Up
actions can be set up underPreferences
->Java
->Code Style
->Cleanup
and they are almost the same as the save actions, but their is no common way to set them..xml
file that can be imported under the cleanup-settings from our.epf
file.Source
->Clean up
, which should apply our save actions on multiple files automatically.