-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Lab: Use Results to Avoid Warning #9156
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
base: main
Are you sure you want to change the base?
Conversation
| bool success = script_file.open(QIODevice::ReadOnly); | ||
| if((! success) || (!script_file.isReadable())) { |
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.
Why, wouldn't :
if (!script_file.open(QIODevice::ReadOnly))
be sufficient?
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.
I have no idea if in Qt open means readable, and wanted to make a minimal code change. Maybe it is redundant, but at least the warning should be gone.
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.
I think it will be readable seen the used flag.
Indeed a small change is preferable but wouldn't
if (!script_file.open(QIODevice::ReadOnly) || !script_file.script_file.isReadable())
also be considered a small change (I would)
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.
I do not understand. Is this a typo or word play ?
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.
I don't see a typo, I did edit the response though as at first there were 2 identical conditions (that are shown in the email).
My last suggestion is just the fact that the extra variable and line is not needed is not needed.
The usage of the variable also indicates a bit, at least to me, that it might be used later on as well
| bool success = output.open(QIODevice::WriteOnly | QIODevice::Text); | ||
|
|
||
| if(!output.isOpen()){ | ||
| if((! success) || (!output.isOpen())){ |
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.
Wouldn't
if (!script_file.open(QIODevice::ReadOnly))
be sufficient?
Summary of Changes
Check that
open()worked to avoid warnings here.Release Management
Affected package(s): Lab
License and copyright ownership: unchanged