Use DMD in NumberStyleCheck #88
Conversation
edi33416
force-pushed
the
replace_libdparse
branch
from
820a5a8
to
2bf728f
Compare
Vladiwostok
force-pushed
the
number-style
branch
from
f641aec
to
e67fc4f
Compare
Vladiwostok
force-pushed
the
number-style
branch
from
e67fc4f
to
f39b908
Compare
|
|
||
| __gshared ErrorSinkNull errorSinkNull; | ||
| if (!errorSinkNull) | ||
| errorSinkNull = new ErrorSinkNull; |
There was a problem hiding this comment.
Why are we doing all of these initializations here? Also, why is the file read here?
There was a problem hiding this comment.
Nvm, I understand now. Isn't there any way to reuse a prior used errorSinkNull? For example, the one that was used to lex the initial source?
There was a problem hiding this comment.
Isn't that the behaviour of __gshared attribute?
| @@ -1350,6 +1346,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN | |||
| config.redundant_storage_classes == Check.skipTests && !ut | |||
| ); | |||
|
|
|||
| if (moduleName.shouldRunDmd!(NumberStyleCheck!ASTCodegen)(config)) | |||
There was a problem hiding this comment.
Why is ASTCodegen passed here since we are only using the lexer?
There was a problem hiding this comment.
Because the BaseAnalyzerDmd follows an inheritance chain starting with the parent class ParseTimeVisitor!ASTCodegen . Should ASTBase struct be used instead?
There was a problem hiding this comment.
Hm, it's kind of annoying that BaseAnalyzerDmd is hardcoded to ASTCodegen. Why not just pass AST to BaseAnalyzer?
E.G:
class BaseAnalyzerDmd(AST) : ParseTimeVisitor!AST {...}I suggest you look into this in a subsequent PR.
| ^^^^^^^ [warn]: Use underscores to improve number constant readability. +/ | ||
| a = 10000; // [warn]: Use underscores to improve number constant readability. | ||
| a = 100000; // [warn]: Use underscores to improve number constant readability. | ||
| a = 1000000; // [warn]: Use underscores to improve number constant readability. |
There was a problem hiding this comment.
I would also add some numbers with underscores just to make sure that does do not actually output any warnings. E.G: 1_000. Also, should 10_00 output any warnings?
There was a problem hiding this comment.
Will add tests for passing cases.
Judging from the existing bad decimal regex (5 or more decimals), 10_00 should not output any warnings
| addErrorMessage(t, "dscanner.style.number_literals", | ||
| "Use underscores to improve number constant readability."); | ||
| } | ||
| auto bytes = readFile(fileName) ~ '\0'; |
There was a problem hiding this comment.
Is it really necessary to reread the file? Don't we already have the source in memory?
There was a problem hiding this comment.
I don't think so. I'm thinking a proper solution for this would be a bigger refactor - a common lexer visitor base class, passing down a token stream to the visitors created from the initial d-scanner run, etc.
No description provided.