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

QL: improve the "this block-comment should have been a QLDoc"-query #11294

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

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Nov 16, 2022

This would have caught a recent QLDoc check that I spent way too long trying to fix, where I didn't realise that the top-level comment was not a QLDoc.

I also fixed a whole bunch of the issues found by the query.
Not all, some results are benign.

And also some improvements to how we match up QLDoc in QL-for-QL.

ql/ql/src/codeql_ql/ast/Ast.qll Fixed Show fixed Hide fixed
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Nov 16, 2022
@erik-krogh erik-krogh marked this pull request as ready for review Nov 16, 2022
@erik-krogh erik-krogh requested review from a team as code owners Nov 16, 2022
Copy link
Contributor

@michaelnebel michaelnebel left a comment

C# LGTM

Copy link
Contributor

@geoffw0 geoffw0 left a comment

C++ differences LGTM.

node instanceof TopLevel and
comment = getCommentAtStart(node.getLocation().getFile(), 1) and
nodeDescrip = "the file"
) and
Copy link
Contributor

@geoffw0 geoffw0 Nov 17, 2022

Choose a reason for hiding this comment

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

It feels like we could remove some redundant logic here by adding a reason parameter to getACommentThatCouldBeQLDoc returning "the below code" or "the file".

Copy link
Contributor Author

@erik-krogh erik-krogh Nov 17, 2022

Choose a reason for hiding this comment

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

Yeah, that could save some duplication.
But that sounds like something for a followup, this seems good enough to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants