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

Rename DominatorsAnalysis to reflect exception handling strategy #1741

Merged
merged 9 commits into from
Nov 4, 2021

Conversation

gamesh411
Copy link
Contributor

@gamesh411 gamesh411 commented Oct 6, 2021

Dominators analysis available thru ClassContext performs an analysis that
disregards exception edges. References to DominatorsAnalysis were renamed to
reflect this.

Make ClassContext API for DominatorsAnalysis more descriptive

Dominators and PostDominators analyses are made available via
ClassContext API in exception-ignoring and implicit exception-ignoring
variants.


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

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

  Dominators analysis available thru ClassContext performs an analysis that
  disregards exception edges. References to DominatorsAnalysis were renamed to
  reflect this.

Make ClassContext API for DominatorsAnalysis more descriptive

  Dominators and PostDominators analyses are made available via
  ClassContext API in exception-ignoring and implicit exception-ignoring
  variants.
@gamesh411
Copy link
Contributor Author

Hey!
Regarding this patch:
I have noticed that spotbugs/src/sampleXml/analysisResultsWithFilterAndUserAnnotations.xml references these classes. I have introduced new classes and I don't know how to generate that XML file to update references inside the sample XML. I would gladly update those as well if you could point me in the right direction as to how the contents of the XML is generated.
Thanks :)

@gamesh411
Copy link
Contributor Author

Thanks for the quick response! Fixed the license.
BTW, do you prefer if I just add my fixes as separate commit (like I have done now) and later squish the commits of the PR, or should I just rebase and force push as necessary?

@ThrawnCA
Copy link
Contributor

Thanks for the quick response! Fixed the license. BTW, do you prefer if I just add my fixes as separate commit (like I have done now) and later squish the commits of the PR, or should I just rebase and force push as necessary?

Please don't force-push, it makes a mess.

ThrawnCA
ThrawnCA previously approved these changes Oct 13, 2021
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

Please add an entry in the CHANGELOG.md to tell renamed classes, it could be necessary for plugin developers.

@gamesh411
Copy link
Contributor Author

Please add an entry in the CHANGELOG.md to tell renamed classes, it could be necessary for plugin developers.

Good point, I have summarized the naming changes in CHANGELOG.md.

CHANGELOG.md Outdated
@@ -8,6 +8,10 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
### Changed
- Replace "分析" with "解析" in Japanese document ([#1573](https://github.com/spotbugs/spotbugs/issues/1573))
- Add a section to document how to integrate find-sec-bugs into spotbugs-maven-plugin ([#540](https://github.com/spotbugs/spotbugs/issues/540))
- Changes related to dominators analysis in package `edu.umd.cs.findbugs.classfile.engine.bcel`:
- `DominiatorsAnalysisFactory` renamed to `NonExceptionDominatorsAnalysisFactory` (clarification)
Copy link
Contributor

Choose a reason for hiding this comment

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

Misspelled "Dominators" (and yes, the original was correctly spelled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, spelling is hard (for me at least) :)
Ironically, while trying to fix one spelling error, I introduced another one.
(not to mention that the lesson learned here contradicts my current efforts on using less copy-paste in general xD)

@KengoTODA KengoTODA mentioned this pull request Nov 4, 2021
@KengoTODA KengoTODA merged commit d63e473 into spotbugs:master Nov 4, 2021
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.

None yet

3 participants