Fixes cases of false or non positive with the useless init check · Pull Request #475 · dlang-community/D-Scanner

ghost · 2017-06-22T18:15:11Z

This PR fixes 4 bugs found while applying the useless initializer check to phobos.

ghost · 2017-06-23T00:48:03Z

Right now with these fixes i just have 1 warning in phobos.

./std/conv.d(4385:28)[warn]: Variable init initializer is useless because it does not differ from the default value

But of course also after a couple of changes in phobos: https://github.com/BBasile/phobos/commit/0994227197b7a27dfc879466973470758072b35c

ghost · 2017-06-23T02:59:03Z

This is good now. After this phobos PR and if you run

dscanner -S | grep "initializer is useless" > dscanner.txt

in phobos repository then a 0 Kio file is produced.

wilzbach

Is there any reason why you seem to prefer filter!(lambda).empty over canFind!lambda?

In general this LGTM.

wilzbach · 2017-06-25T11:11:17Z

src/analysis/useless_initializer.d

+    DynamicArray!(string) _structStack;
+    DynamicArray!(bool) _inStruct;
+    DynamicArray!(bool) _atDisabled;
+    DynamicArray!(bool) _inTest;


nit: Hmm I am not a huge fan of using _ if there's a proper type system with private in place.

wilzbach · 2017-06-25T11:12:48Z

src/analysis/useless_initializer.d

+
+    override void visit(const(StructDeclaration) decl)
+    {
+        if (_inTest.back())


If you always just test the end of the array, wouldn't a simple bool flag suffice?

because of this case:

unittest{struct A{unittest{}}}

wilzbach · 2017-06-25T11:15:41Z

src/analysis/useless_initializer.d

+            // initializer has to appear clearly in generated ddoc
+            decl.comment !is null ||
+            // issue 474, manifest constants HAVE to be initialized.
+            !decl.storageClasses.filter!(a => a.token == tok!"enum").empty)


How about using canFind?

wilzbach · 2017-06-25T11:17:37Z

src/analysis/useless_initializer.d

 import dparse.ast;
 import dparse.lexer;
+import std.algorithm : among, canFind;
+import std.algorithm.iteration : filter;


Why not simply importing entire std.algorithm - it's very commonly used... and saves the pain of updating this list all the time.

wilzbach · 2017-06-25T11:21:11Z

src/analysis/useless_initializer.d

                enum warn = q{addErrorMessage(declarator.name.line, declarator.name.column,
                    key, msg);};
+            }
            else


Off-topic: I am not a huge friend of this pattern. While I do the see merits of being able to just use X, the string formatting could (in theory of course) be broken.

wilzbach · 2017-06-25T11:28:51Z

src/analysis/useless_initializer.d

+        if (_inStruct.length > 1 && _inStruct[$-2] &&
+            ((decl.parameters && decl.parameters.parameters.length == 0) || !decl.parameters))
+        {
+            _structCanBeInit[_structStack.back()] = !_atDisabled[$-1];


While it shouldn't matter, we could save the duplicate hashmap query here:

bool canBeInit = !_atDisabled[$-1]; if ... _structCanBeInit[_structStack.back()] = canBeInit

wilzbach · 2017-06-25T11:34:01Z

src/analysis/useless_initializer.d

    static immutable intDefs = ["0", "0L", "0UL", "0uL", "0U", "0x0", "0b0"];

+    HashMap!(string, bool) _structCanBeInit;
+    DynamicArray!(string) _structStack;


As mentioned below: I think _structStack and _atDisabled don't need to be arrays.

wilzbach · 2017-06-25T11:41:09Z

src/analysis/useless_initializer.d

+        if (_inTest.back())
+            return;
+
+        _structStack.insert(decl.name.text);


Hmm are you sure this would be unique? Wouldn't you need proper mangling?
Consider:

struct F { struct G {} G g; } struct G { struct G {} // collision G g; }

I'll use the FQN

… useless init check

- FQN the struct names - prevent a double query in the canBeInit AA - import the whole also package - there was not test on non-initilized variables

skl131313 · 2017-06-25T23:14:22Z

src/analysis/base.d

 protected:

-	bool inAggregate = false;
+	bool inAggregate;


Looks like there's a tab here instead of spaces, even before the change. Might be a good idea to setup some code guidelines and checks for them, not sure if there were any before.

For sure there's one. @Hackerpilot uses tabs on his stuff. Excepted libdparse because it was planned to be included in phobos.

The .editorconfig file for this project does specify tabs.

wilzbach

LGTM.

Small nits (I don't care about them)

_atDisabled and _structStack could be replaced by a flag?
weird whitespace in duplicate_attribute.d

wilzbach · 2017-06-28T04:50:09Z

src/analysis/duplicate_attribute.d

-		bool hasNoThrow = false;
+		bool hasProperty;
+		bool hasSafe ;
+		bool hasTrusted ;


I guess the whitespace before ; is accidental?

wilzbach · 2017-06-28T05:01:29Z

src/analysis/useless_initializer.d

+        {
+            _atDisabled[$-1] = decl.attributes
+                .canFind!(a => a.atAttribute !is null && a.atAttribute.identifier.text == "disable");
+        }


OT: At some point we probably should start sharing code between the checkers. I imagine sth. like isInDisabledStruct to be a fairly common request.

wilzbach · 2017-06-28T05:28:31Z

src/analysis/useless_initializer.d

-                continue;
-            if (!declarator.initializer || !declarator.initializer.nonVoidInitializer)
-                continue;
+            return;


Speaking of tabs, this looks like whitespace here. Should we maybe enable a checker on Travis, s.t. we can't push files which are beginning with tabs?

(Sorry that I didn't realize this earlier - I don't care about the holy war of tabs vs. whitespace, but we should try to keep this repo consistent)

I'm gonna patch this. I still have to add editorconfig support in Coedit. Currently the tab style is autodetected and for a new module the user preference is used, so spaces here.

dlang-bot · 2017-06-28T05:42:05Z

Thanks for your pull request, @bbasile!

wilzbach · 2017-06-28T05:49:35Z

I'm gonna patch this. I still have to add editorconfig support in Coedit. Currently the tab style is autodetected and for a new module the user preference is used, so spaces here.

Ok. The history got moderately long for this diff. Do you want to squash everything before merging?
(I don't mind either way).
Btw should we define a guideline for this at https://github.com/dlang-community/discussions?

ghost · 2017-06-28T05:57:41Z

Yes squash. If GH wouldn't allow to do it i would have rebased and marked fixup all the commits.

…ng-community#475) * fix dlang-community#474 fix dlang-community#473 fix dlang-community#476 - Cases of false and non positive with the useless init check * do not warn on documented variables * fix dlang-community#477 - Custom type initialized to init should not trigger a warn * allow struct.init when know struct has `@disable` ctor * fix last false detection in phobos * prevent check in the "compiles" trait * - use canFind when filter.empty was negated - FQN the struct names - prevent a double query in the canBeInit AA - import the whole also package - there was not test on non-initilized variables * fix, self-linting missed a case that was not yet fixed * fix more undetected warns during self linting * use a flag instead of a stack + apply skipTests * convert spaces to tabs

ghost changed the title ~~fix #474 - Case of false positive with the useless init check : manifest constants~~ fix #474 fix #475 - Cases of false positive with the useless init check Jun 22, 2017

ghost changed the title ~~fix #474 fix #475 - Cases of false positive with the useless init check~~ fix #474 fix #473 - Cases of false positive with the useless init check Jun 22, 2017

ghost force-pushed the issue-474 branch 3 times, most recently from 768325b to 106806e Compare June 22, 2017 19:10

ghost changed the title ~~fix #474 fix #473 - Cases of false positive with the useless init check~~ fix #474 fix #473 fix #475 - Cases of false and non positive with the useless init check Jun 22, 2017

ghost force-pushed the issue-474 branch from 1bf6068 to 2c3eff7 Compare June 22, 2017 19:41

ghost changed the title ~~fix #474 fix #473 fix #475 - Cases of false and non positive with the useless init check~~ [WIP ] Fixes cases of false or non positive with the useless init check Jun 22, 2017

ghost changed the title ~~[WIP ] Fixes cases of false or non positive with the useless init check~~ Fixes cases of false or non positive with the useless init check Jun 23, 2017

ghost mentioned this pull request Jun 25, 2017

Run DScanner on itself #479

Merged

wilzbach reviewed Jun 25, 2017

View reviewed changes

ghost force-pushed the issue-474 branch from 8930146 to 313e343 Compare June 25, 2017 22:12

Basile Burg added 9 commits June 26, 2017 00:36

fix #474 fix #473 fix #476 - Cases of false and non positive with the…

a1ed66d

… useless init check

do not warn on documented variables

191a694

fix #477 - Custom type initialized to init should not trigger a warn

ea70c61

allow struct.init when know struct has @disable ctor

5d41fd9

fix last false detection in phobos

133e3b9

prevent check in the "compiles" trait

12c92b7

- use canFind when filter.empty was negated

b37b398

- FQN the struct names - prevent a double query in the canBeInit AA - import the whole also package - there was not test on non-initilized variables

fix, self-linting missed a case that was not yet fixed

7d01a74

fix more undetected warns during self linting

7ef6c4b

ghost force-pushed the issue-474 branch from b8bf97c to 7ef6c4b Compare June 25, 2017 22:39

ghost deleted a comment from dlang-bot Jun 25, 2017

skl131313 reviewed Jun 25, 2017

View reviewed changes

ghost mentioned this pull request Jun 28, 2017

Merge master into phobos #482

Merged

wilzbach approved these changes Jun 28, 2017

View reviewed changes

use a flag instead of a stack + apply skipTests

49dbb33

ghost deleted a comment from dlang-bot Jun 28, 2017

wilzbach reviewed Jun 28, 2017

View reviewed changes

convert spaces to tabs

e9b792a

ghost merged commit f89d356 into master Jun 28, 2017

ghost deleted the issue-474 branch July 5, 2017 07:36

This pull request was closed.

Conversation

ghost commented Jun 22, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 23, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 23, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skl131313 Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Jun 28, 2017

Uh oh!

wilzbach commented Jun 28, 2017

Uh oh!

ghost commented Jun 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Jun 22, 2017 •

edited by ghost

Loading

ghost commented Jun 23, 2017 •

edited by ghost

Loading

ghost commented Jun 23, 2017 •

edited by ghost

Loading

wilzbach Jun 25, 2017 •

edited

Loading

wilzbach Jun 25, 2017 •

edited

Loading

skl131313 Jun 25, 2017 •

edited

Loading