[CPP-386] More getCanonicalQlClass() goodness

[zlaski-semmle]

The new node types were uncovered by running the following query against torvalds/linux, vim/vim and wireshark/wireshark:

import cpp

from Element e
where not exists(string s | s = e.getCanonicalQLClass() | s != "???")
select e, e.getLocation(), concat(e.getAQlClass(), ", ")

I'm still trying to create test cases for these; the LGTM console is less than helpful.

The merge conflict is with #1647 and is trivial to resolve.

[jbj]

LGTM except for two comments.

[jbj]

Why add the macro name to the string? I don't see the value, but it might hurt performance, and it seems inconsistent with all the other getCanonicalQLClass overrides. If someone is looking for a string that identifies this, they already have toString.

[jbj]

Typo: PreprocessorIfsef -> PreprocessorIfdef.

[jbj]

I haven't been part of the latest "what's canonical" discussions, so there might be additional feedback from those who were. @dave-bartolomeo ?

[geoffw0]

Changes LGTM. Had a go with this on a couple of projects locally - it's a big improvement as far as completeness is concerned. 👍

If we're looking for further improvements:

• ConstructorInit, MetricFile, MicrosoftTryExceptStmt still have no getCanonicalQLClass()
• Literal + Zero occur together frequently
• there are also some overlaps to do with templates, but I think they're fairly natural and it might not actually help anyone to resolve them.

[geoffw0]

Why do we include additional information just for Macros, MacroInvocations, Specifiers and a few other things?

[jbj]

@geoffw0 We don't want to overrides for MetricFile and its cousins. Every File is a MetricFile.

[geoffw0]

We don't want to overrides for MetricFile and its cousins. Every File is a MetricFile.

Good point. I was seeing some Files with no getCanonicalQLClass(), but MetricFile is indeed the wrong choice for them. I think a File in the snapshot that is neither a HeaderFile, CFile or CppFile should have getCanonicalQLClass() of just "File".

[jbj]

Otherwise LGTM

[jbj]

There are still some attributes and specifiers that need to have this parenthesis removed. Search for ")" in the diff to find the others.

[jbj]

@zlaski-semmle, don't forget to rebase so the tests can run.

[geoffw0]

This should be "ConstructorInit".

[geoffw0]

Apart from the comment I've just made, my concerns have been addressed.

[geoffw0]

Something's gone wrong with a merge here. I would suggest you try a "rebase" instead of merging next time you have a conflict on a PR.

[zlaski-semmle]

Something's gone wrong with a merge here. I would suggest you try a "rebase" instead of merging next time you have a conflict on a PR.

I thought I did rebase (I used the git rebase command), but the result surprised me also. Perhaps you could provide herein the exact incantation to rebase, say , branch zlaski/cpp386?

My mistake aside, is zlaski/cpp386 mergeable as is?

[geoffw0]

Perhaps you could provide herein the exact incantation to rebase, say , branch zlaski/cpp386?

I'd use git rebase --onto upstream/master zlaski/cpp386~X zlaski/cpp386, where X is the number of commits from the top to rebase (there are shorter forms of this command, but this one should do what you want without surprises).

[zlaski-semmle]

Perhaps you could provide herein the exact incantation to rebase, say , branch zlaski/cpp386?

I'd use git rebase --onto upstream/master zlaski/cpp386~X zlaski/cpp386, where X is the number of commits from the top to rebase (there are shorter forms of this command, but this one should do what you want without surprises).

Looking at the commits for this PR, I now see that my commits are interspersed with others' commits that I pulled in with my failed rebase

Perhaps you could provide herein the exact incantation to rebase, say , branch zlaski/cpp386?

I'd use git rebase --onto upstream/master zlaski/cpp386~X zlaski/cpp386, where X is the number of commits from the top to rebase (there are shorter forms of this command, but this one should do what you want without surprises).

I tried the the git rebase command you suggested but got a bunch of weird conflicts in C#, Python, etc. (Perhaps I misunderstood what the value X should actually be.) So I did a hard rewind followed by 2 cherry-picked commits. This should be good to merge in.

[jbj]

LGTM, but a test is failing. It looks like it's just a matter of accepting the new output.

[zlaski-semmle]

LGTM, but a test is failing. It looks like it's just a matter of accepting the new output.

It's a strange beast. At any rate, I'm adding a direct link to the offending file here:
https://github.com/zlaski-semmle/ql/blob/zlaski/cpp386/cpp/ql/test/library-tests/ir/ir/PrintAST.expected

[zlaski-semmle]

All right, I finally squashed the Jenkins test failure. The PR should be ready to go.

[jf205]

@zlaski-semmle are the changes to docs/language/learn-ql/cpp/introduce-libraries-cpp.rst intended to be here, or has #1658 mistakenly been merged into this PR?

[zlaski-semmle]

@zlaski-semmle are the changes to docs/language/learn-ql/cpp/introduce-libraries-cpp.rst intended to be here, or has #1658 mistakenly been merged into this PR?

The latter, I'm afraid. I rebased the #1658 PR (successfully, it would appear) but then that spilled into this PR: somehow.

[zlaski-semmle]

I made a total mess of things here, so needed to create a new PR:: #1753. Sorry for the inconvenience.