Skip to content
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

Fix FPs in CT_CONSTRUCTOR_THROW when the finalizer does not run #2716

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

JuditKnoll
Copy link
Collaborator

This PR fixes #2710.


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

@hazendaz
Copy link
Member

We should automate the changelog entry somehow from an action, its always conflicting

@iloveeclipse
Copy link
Member

Because of the merge in between Merge branch 'master' into ct_fix_2710 this PR can't be rebased.

I've fixed that & properly rebased the change plus fixed inconsistencies in the current changelog.
Will push on the branch in a moment.

@iloveeclipse iloveeclipse merged commit e847d62 into spotbugs:master Nov 28, 2023
6 checks passed
@JuditKnoll
Copy link
Collaborator Author

JuditKnoll commented Nov 28, 2023

Because of the merge in between Merge branch 'master' into ct_fix_2710 this PR can't be rebased.

I've fixed that & properly rebased the change plus fixed inconsistencies in the current changelog. Will push on the branch in a moment.

@iloveeclipse Isn't a merging the master to the feature branch generally preferred, than rebasing to master? Since the latter changes the commit ids, makes the review a bit more difficult (the changes since the last review can't be reviewed easily), and requires a force push, also it's easier for less experienced git users to delete changes and make a mess. I would love to hear your point on this.

@JuditKnoll JuditKnoll deleted the ct_fix_2710 branch November 28, 2023 10:38
@iloveeclipse
Copy link
Member

iloveeclipse commented Nov 28, 2023

I always prefer rebase and avoid using merge if possible, also for non trivial PR's I always use IDE and not web UI.
There are few items that make "merge" not nice:

  • "Rebased" work is much easier to follow / test single changes.
  • Rebasing merge commits is not possible at all.
  • Reviewing merge commits is a mess, as that hides the actual changes behind potentially huge number of changes.
  • One can always "re-apply" non-merge commits to any branch to see what the change would do there - not possible with merge commits
  • May be more here.

So from my point of view (I spend 90% of my work time on reviews) merge is a biggest possible PITA.

@hazendaz
Copy link
Member

hazendaz commented Nov 28, 2023 via email

@hazendaz hazendaz added this to the SpotBugs 4.8.2 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive CT_CONSTRUCTOR_THROW on canonical "Compliant Solution"
4 participants