KEMBAR78
Fix lint errors emitted by HHClientLinter by lexidor · Pull Request #406 · hhvm/hhast · GitHub
Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

Conversation

lexidor
Copy link
Contributor

@lexidor lexidor commented Nov 25, 2021

Intentionally skipped three, see hhast-lint.json

The hh_client --lint linter catches far more cases.
Interface override, override from traits are those I took note of.
The codegen directory was intentionally ignored in this PR.
Required some special casing in CodegenSyntax.
Codegen lint errors are intentionally ignored in this commit.
This commit will fail CI because of that.
I confirmed that this is correct by doing codegenning this first.
$check = (Node $node) ==> $node; $check($this->_the_thing); return true;
This did not generate type errors for passing a nullable for Node.
This will become problematic when reference (object) typed properties are added.
All properties are primitive, structural, or value arrays as of November 2021.
Calling the constructor explcitly would cost a lot of needless re-`Vec\sort_by()` work.
HHAST_FIXME[5562] */
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we let schema be a memoized function to avoid recalculation of Vec\sort_by

Copy link
Contributor

Choose a reason for hiding this comment

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

Schema's rather large, and memoize is wasteful

The more common way to address this is to make the constructor protected, and move the expensive computation that's currently in the constructor to a public static method

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently withoutHackfmt is called immediately after a CodegenBase is created, therefore the constructor will be called twice if we remove the clone operator. Memoization would not cost more memory than the current implementation because anyway schema would be stored on CodegenBase. The problem the that memoization could solve is to avoid the schema calculation in the first call to the constructor, because the fist instance can be discarded before the memoized function getting executed.

@Atry Atry merged commit 34ab5a2 into hhvm:main Nov 25, 2021
@fredemmott
Copy link
Contributor

This is going to need cleanup once #408 is resolved

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants