Skip to content

C++: clearer alert wording in integer-multiplication-cast-to-long#145

Merged
geoffw0 merged 2 commits into
github:masterfrom
nickrolfe:rewording
Sep 4, 2018
Merged

C++: clearer alert wording in integer-multiplication-cast-to-long#145
geoffw0 merged 2 commits into
github:masterfrom
nickrolfe:rewording

Conversation

@nickrolfe
Copy link
Copy Markdown
Contributor

@nickrolfe nickrolfe commented Sep 3, 2018

I always find myself having to re-read this alert a few times before I understand it. I think this is because:

  1. most other alerts describe the problem first, not the solution.
  2. It's ambiguous without further context whether the 'cast' in 'cast to X' is a verb or a noun .

Perhaps it's just me, but I think with this rephrasing it's easier to understand.

@nickrolfe nickrolfe added the C++ label Sep 3, 2018
@nickrolfe nickrolfe requested a review from jbj September 3, 2018 16:05
jbj
jbj previously approved these changes Sep 4, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

I agree that the new wording is better. Any comments from @kevinbackhouse or @geoffw0 ?

@geoffw0
Copy link
Copy Markdown
Contributor

geoffw0 commented Sep 4, 2018

most other alerts describe the problem first, not the solution.

Your change makes the message a bit long, but it's clearer so I think overall that's an improvement. I'll ping Alistair in case he has any suggestions.

(also there are some tips on https://wiki.semmle.com/display/IN/Writing+queries+and+query+help#Writingqueriesandqueryhelp-Alertquerymessages).

@semmledocs-ac
Copy link
Copy Markdown
Contributor

semmledocs-ac commented Sep 4, 2018

The advice on the wiki https://wiki.semmle.com/display/IN/Writing+queries+and+query+help#Writingqueriesandqueryhelp-Alertquerymessages
says:
Don't suggest how to fix a problem in an alert, leave that to the help. For example, if the message is "Declaration 'foo' shadows existing declaration (foo)" then refrain from adding "consider renaming one of the variables".
which would suggest omitting the second sentence:
"Consider casting before multiplication to avoid potential overflow."

@nickrolfe
Copy link
Copy Markdown
Contributor Author

nickrolfe commented Sep 4, 2018

Thanks. I've removed the second sentence.

@geoffw0
Copy link
Copy Markdown
Contributor

geoffw0 commented Sep 4, 2018

Great, merging...

@geoffw0 geoffw0 merged commit ae80b8d into github:master Sep 4, 2018
@nickrolfe nickrolfe deleted the rewording branch September 4, 2018 17:01
aibaars added a commit that referenced this pull request Oct 14, 2021
@aibaars
Merge pull request #145 from github/aibaars/fix
ce69c91 
Fix regression in rb/unused-parameter
smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021
@igfoo
Merge pull request github#145 from github/igfoo/todo
49e78a9 
Kotlin: TODO() now works
MathiasVP added a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025
@MathiasVP
Merge pull request github#145 from microsoft/jb1/isLibraryFile-nomagic
029fb0b 
No magic on `ClassifyFiles::isLibraryFile`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbj jbj jbj approved these changes

+1 more reviewer

@semmledocs-ac semmledocs-ac semmledocs-ac approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nickrolfe @geoffw0 @semmledocs-ac @jbj