GHA CI: add CodeQL scanning

[Chocobo1]

• GHA CI: add CodeQL scanning
  This enable codebase scanning for C++ and JavaScript languages.
  https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/about-code-scanning-with-codeql
  https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/customizing-code-scanning

• GHA CI: drop needless syntax
  https://docs.github.com/en/actions/learn-github-actions/expressions#about-expressions

  When you use expressions in an if conditional, you may omit the
  expression syntax (${{ }}) because GitHub automatically evaluates the if conditional as an expression.

• Fix code defects

[Chocobo1]

This has too many false-positives at the moment so exclude it for now.

[glassez]

And there are many warnings that look like false positives. What do you suggest to do with them?

[glassez]

Why to check it if you assigned it to undefined just above?

[Chocobo1]

I can change it to var keyboard; but I wanted to be explicit.

[Chocobo1]

CodeQL report the variable wasn't declared before use.

[glassez]

CodeQL report the variable wasn't declared before use.

Although I am not familiar enough with JS stuff, but to me it looked as if keyboard was intended to preserve a previously assigned value (i.e., to reuse Keyboard object once created).

[glassez]

Otherwise it is meaningless to check variable just declared as undefined, isn't it?

[glassez]

since the variable is still declared in the scope in which it is used.

This is the confusing point which my previous comment aims to address.

It still looks to me like an attempt to give a slightly less confusing look to something that is confusing by nature. But I won't go into further discussions about it. It is enough that now (after applying var keyboard;) the "fixing the analyzer warnings" does not break the logic of the program.

[Chocobo1]

This is the confusing point which my #18687 (comment) aims to address.

commit updated.

[Chocobo1]

It still looks to me like an attempt to give a slightly less confusing look to something that is confusing by nature.

Yes and in the future with the analyzer in place, new code can be inspected better or even rewritten to avoid such situations.

[loligans]

yeah it's pretty confusing. i believe @glassez is correct in that you can redeclare var keyboard

[HanabishiRecca]

This is a very, very bad and confusing practice. It is better to avoid legacy JS mistakes and do something like

let { keyboard } = globalThis;
if (!keyboard) {
    keyboard = new Keyboard({

[Chocobo1]

What do you suggest to do with them?

Why don't you just state your intend? I would suppress the warnings with the most false positives and keep the lesser ones. The remaining false positives can be dismissed by hand.

[Chocobo1]

I would suppress the warnings with the most false positives and keep the lesser ones. The remaining false positives can be dismissed by hand.

After suppressing some CodeQL checks, the final report looks clean now.

[glassez]

What do you suggest to do with them?

Why don't you just state your intend?

I didn't have clear intentions, I just saw a picture that I didn't like. Using analyzers that give a lot of false positives can have a negative effect. I suppose you've at least got a little familiar with CodeQL, since you're offering it. So I would expect it to be configured as much as possible before we merge it (assuming it can be configured appropriately).

[Chocobo1]

What do you suggest to do with them?

Why don't you just state your intend?

Using analyzers that give a lot of false positives can have a negative effect. I suppose you've at least got a little familiar with CodeQL, since you're offering it. So I would expect it to be configured as much as possible before we merge it (assuming it can be configured appropriately).

Of course and I meant you could just say that (wished for less false positives) instead of asking my opinion which I find it to be ambiguous.

Also IMO the CodeQL is more useful for the js language than for c++ since there wasn't any good static analyzer employed. As for c++ just think of it as another checker for reference, it might be occasionally useful and it won't (shouldn't) be a hard block for merging PRs.