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

Jump to bottom

fix request for cpp exceptions #7177

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix request for cpp exceptions #7177

wants to merge 5 commits into from
+3 −9

Conversation

@ihsinme
Copy link
Contributor

@ihsinme ihsinme commented Nov 19, 2021

good day @geoffw0
I want to make changes to this request.
to qualify for the reward program, thank you.
@ihsinme ihsinme requested a review from as a code owner Nov 19, 2021
@github-actions github-actions bot added the C++ label Nov 19, 2021
@ihsinme ihsinme mentioned this pull request Nov 19, 2021
Open
1 task
@JarLob
Copy link
Contributor

@JarLob JarLob commented Nov 19, 2021 •
edited
Loading

Thanks!
Few more FP patterns observed:

  1. An odd, but common usage of an exception for getting the message from what function. Could it be excluded?
  2. Storage of an exception in a wrapper exception and sometimes in a class that is not derived from exception.

Loading

@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Nov 19, 2021

Thanks.
I will definitely try to fix everything.

Loading

@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Nov 19, 2021

@JarLob
even if this is a strong enough restriction.
but we will get rid of all false positives.

Loading

geoffw0
geoffw0 reviewed Nov 19, 2021
View changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment •
edited
Loading

LGTM.

We lose 1 good result (in godotengine/godot), where a std::exception is created, converted to int and returned - which does not seem very sensible. All the other results removed by this change look like they were false positives. I think that's a good tradeoff!

👍

Loading

@JarLob
Copy link
Contributor

@JarLob JarLob commented Nov 19, 2021

Loading

@JarLob
Copy link
Contributor

@JarLob JarLob commented Nov 22, 2021

Results looks good. @geoffw0 @MathiasVP Do you think it is worth to change the message to something like "Object creation of exception type on stack. Did you forget the throw keyword?" Anyway I'm ok with the old message too.

Loading

@MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 22, 2021

Object creation of exception type on stack. Did you forget the throw keyword?

I definitely prefer "Object creation of exception type on stack. Did you forget the throw keyword?" 👍

Loading

@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Nov 23, 2021

@MathiasVP
I made changes.
can you run a check?

Loading

@JarLob
Copy link
Contributor

@JarLob JarLob commented Nov 23, 2021

I made changes. can you run a check?

Done.

Loading

@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Nov 24, 2021

maybe something else is needed from me for this PR to be accepted?

Loading

@MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 24, 2021

This all looks good to me. I'd be happy to merge this as soon as @JarLob is happy with it.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MathiasVP MathiasVP

@geoffw0 geoffw0

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
C++
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@ihsinme @JarLob @MathiasVP @geoffw0