Skip to content

Fix compile warnings#558

Merged
jbreue16 merged 18 commits intomasterfrom
fix/compile-warnings
Mar 17, 2026
Merged

Fix compile warnings#558
jbreue16 merged 18 commits intomasterfrom
fix/compile-warnings

Conversation

@freeware-superman
Copy link
Copy Markdown
Contributor

This PR resolves all issues leading to compile warnings. Already excluded warnings do not get resolved. -Wsign-compare will be handled in a seperate PR as it needs a lot more changes.
Some smaller issues that are found along the way get resolved as well.

Comment thread src/libcadet/model/GeneralRateModel2D.cpp Outdated
@AntoniaBerger
Copy link
Copy Markdown
Collaborator

Are you planning to change the warning settings so that the unsigned/signed comparison warnings are no longer visible?
This way we could be sure that no other warnings will be added in the meantime?

@jbreue16
Copy link
Copy Markdown
Contributor

Hi, please rebase onto master, i fixed some local variable shadowing issues.
I updated to vs 2026 and it made me do it and two of these were actual bugs..

@freeware-superman freeware-superman force-pushed the fix/compile-warnings branch 5 times, most recently from edb7cb4 to 51fdbb3 Compare February 2, 2026 10:55
Comment thread src/libcadet/model/reaction/CrystallizationReaction.cpp Outdated
@freeware-superman freeware-superman force-pushed the fix/compile-warnings branch 2 times, most recently from 85b5fb1 to d26e514 Compare February 2, 2026 23:02
Comment thread src/libcadet/model/reaction/MichaelisMentenReaction.cpp Outdated
@freeware-superman freeware-superman force-pushed the fix/compile-warnings branch 4 times, most recently from 0befba3 to 76a12c4 Compare February 6, 2026 17:35
Comment thread src/libcadet/model/binding/HICWaterOnHydrophobicSurfacesBinding.cpp Outdated
Copy link
Copy Markdown
Contributor

@jbreue16 jbreue16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, thanks for working on this, seems like many warnings are fixed already !
What warnings are left? I think we should merge this and continue on another PR

@jbreue16
Copy link
Copy Markdown
Contributor

@freeware-superman What warnings are left? I think we should merge this and continue on another PR

@AntoniaBerger
Copy link
Copy Markdown
Collaborator

@freeware-superman What warnings are left? I think we should merge this and continue on another PR

We plan to merge in the end of this week / beginning of next week. @freeware-superman wanted to have look on ubuntu specific warnings.

@jbreue16
Copy link
Copy Markdown
Contributor

@freeware-superman What warnings are left? I think we should merge this and continue on another PR

We plan to merge in the end of this week / beginning of next week. @freeware-superman wanted to have look on ubuntu specific warnings.

Cool !

I think we should keep the commits separated and not squash. One small thing that I don't care too much about: Our standard commit message has a capital first letter

Since C++23, empty spaces between quotation marks and suffixes are
deprecated.
The lines changed were indented in a way that may lead to confusion when
not reading extra carefully.
Since C++20 '[=]' is deprecated. For the original funtionality
'[=, this]' should be used.
Using parentheses in complex statements leads to easier readability.
This also prevents confusion with operator presedence.
The order of initialization matters. It should be the same as statet in
the constructor. Not following that order can lead to members with
unexpected values.
Functions returning only the value of a const variable may not be
declared as const.
Running variables defined outside a loop don't need to be defined again.
Not including the header may lead to undefinded behaviour.
Non-void funtions should always return a value.
@freeware-superman freeware-superman marked this pull request as ready for review March 16, 2026 17:22
Copy link
Copy Markdown
Contributor

@jbreue16 jbreue16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the two nParType checks need to be added again. Other than that, this PR looks good to be merged.
I rly like the commit names and descriptions!

Comment thread src/libcadet/model/parts/ParticleDiffusionOperatorDG.cpp
Comment thread src/libcadet/model/ColumnModel1D.cpp Outdated
Comment thread src/libcadet/model/ColumnModel2D.cpp Outdated
Copy link
Copy Markdown
Collaborator

@AntoniaBerger AntoniaBerger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work, thanks a lot! Looks super clean to me.

Comment thread src/libcadet/model/ColumnModel2D.cpp Outdated
@jbreue16
Copy link
Copy Markdown
Contributor

Ok Im having a couple of minutes while my CI runs so Ill add the two changes, force-push and merge

freeware-superman and others added 6 commits March 17, 2026 14:08
If a body is intenionally empty it should be clearly marked.

If-statement was removed since not required.

Co-authored-by: Jan Breuer <74359367+jbreue16@users.noreply.github.com>
A function labeld as having no exceptions should not have exceptions.
A comparison that can never be true (e.g. if(positive_number < 0) ) a
warning is raised.

Co-authored-by: Jan Breuer <74359367+jbreue16@users.noreply.github.com>
If a return value is intentionally ignored, it should be explicitly
marked in code by casting to void.
A struct with unitialized members may lead to uninteded behaviour.
@jbreue16 jbreue16 force-pushed the fix/compile-warnings branch from ae42d4e to 8b95837 Compare March 17, 2026 13:09
@jbreue16 jbreue16 merged commit 4b2943e into master Mar 17, 2026
4 checks passed
@jbreue16 jbreue16 deleted the fix/compile-warnings branch March 17, 2026 13:43
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants