Run DScanner on itself by wilzbach · Pull Request #479 · dlang-community/D-Scanner

wilzbach · 2017-06-25T08:00:24Z

No description provided.

wilzbach · 2017-06-25T08:01:32Z

src/analysis/imports_sortedness.d


 private:

-	int level = 0;


FWIW I'm not really sold on the useless initializer check, but there weren't that many occurrences.

Speaking of that i still have a PR opened that make the check very reliable. (#475)

TBH, despite the recent discussion, I find D code without explicit initializers (when equal to .init) more clear to read, so LGTM.

@bbasile I'll try to review your PR, but I don't have much knowledge of D-Scanner's internals.

wilzbach · 2017-06-25T08:02:17Z

src/analysis/style.d

-	enum string moduleNameRegex = `^[\p{Ll}_\d]+$`;
+	enum string VarFunNameRegex = `^([\p{Ll}_][_\w\d]*|[\p{Lu}\d_]+)$`;
+	enum string AggregateNameRegex = `^\p{Lu}[\w\d]*$`;
+	enum string ModuleNameRegex = `^[\p{Ll}_\d]+$`;


It's a bit ironic if a checker violates its own checks ;-)

No, manifest constants like enum [type] identifier = <init-expr>;are variables, while enums like enum EnumerationName { member1, meber2 } are aggregates, IMO.

ping, this is the only issue with the source left ;)

PetarKirov · 2017-06-25T08:49:37Z

src/analysis/if_statements.d

+		return formatted == other.formatted
+			&& line == other.line
+			&& column == other.column
+			&& depth == other.depth;


Doesn't this contradict the hash function, because it takes the formatted into account and toHash does not?

PetarKirov · 2017-06-25T08:53:30Z

src/analysis/if_statements.d

+	}
+	size_t toHash() const nothrow
+	{
+		return line * 100_000 + column * 100 + depth;


I think better to rely on the built-in hashing here: https://github.com/dlang/druntime/blob/master/src/core/internal/hash.d

PetarKirov · 2017-06-25T08:57:30Z

src/analysis/if_statements.d

 			return 0;
 		return -1;
 	}
+	bool opEquals(ref const ExpressionInfo other) const nothrow


Actually, are these functions added because of object_const_check? If that's the case I think this is a false positive, since the compiler already generates those functions for structs.

PetarKirov · 2017-06-25T08:59:40Z

src/analysis/pokemon.d

-		auto identOrTemplate = type2.symbol.identifierOrTemplateChain
-			.identifiersOrTemplateInstances[0];
+		const identOrTemplate = type2.symbol.identifierOrTemplateChain.identifiersOrTemplateInstances[0];
 		if (identOrTemplate.templateInstance !is null)


Um, why? This lines is already 106 colons long with the two 4-width tabs.

PetarKirov

Overall look good to me, except where I have commented with an objection.

wilzbach · 2017-06-25T11:04:36Z

src/analysis/unmodified.d

 		if (guaranteeUse == 0)
 		{
-			const r = tree[index].equalRange(&vi);
+			auto r = tree[index].equalRange(&vi);


This lead to the compile error on Travis:

src/analysis/unmodified.d(202): Error: mutable method std.container.rbtree.RBRange!(RBNode!(VariableInfo*)*).RBRange.front is not callable using a const object

I'll submit a Phobos PR for that (I already have the fix locally, but I want to add some tests), but that's the right way to go, since we support multiple versions of Phobos anyway.

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

@ZombineDev

Actually, are these functions added because of object_const_check?

It's due to the opequals_tohash_check, but I think it's better to go with a minimal load of checks in the beginning and discuss on a solution for enabling these checks on a case by case basis ;-)
That's why I removed the controversial bits and set the checks to disabled.
Of course, we should squash all commits into one.

PetarKirov · 2017-06-25T11:16:07Z

It's due to the opequals_tohash_check

Yeah, I was looking for opequals_tohash_check, but somehow I only found object_const_check when searching for toHash. Strange.

That's why I removed the controversial bits and set the checks to disabled.

Yes, we can always discus them later.

PetarKirov · 2017-06-25T11:20:32Z

Of course, we should squash all commits into one.

Yep, no problem, but individual commits are extremely helpful when reviewing ;)

PetarKirov · 2017-06-25T11:23:33Z

makefile

 	rm -f bin/dscanner-unittest

+lint: dmdbuild
+	./dsc --config .dscanner.ini --styleCheck src


./dsc --config .dscanner.ini --styleCheck src ->
./bin/dscanner --config .dscanner.ini --styleCheck src

(There's no dsc as far as I can see.)

PetarKirov · 2017-06-25T11:24:53Z

.travis.sh

 else
    git submodule update --init --recursive
    make test
+    make lint


I heavily prefer if we ran lint on both the ldc, dmd and the dub build.

wilzbach · 2017-06-25T11:53:25Z

I heavily prefer if we ran lint on both the ldc, dmd and the dub build.

Hmm I don't mind, but on my machine it takes ~40s (~1.5min on CircleCI) to do the release build and the source code doesn't change ...
Btw if we already talk about the Makefile I would be strongly in favor of

allowing a "slim" debug build (=debug without logging, at Phobos we already patch DScanner for this as building the debug version is a lot faster)
use non-changing target that don't run when no source file has changed (with the current makefile make lint will always build DScanner)

PetarKirov · 2017-06-25T11:57:44Z

Hmm I don't mind, but on my machine it takes ~40s (~1.5min on CircleCI) to do the release build and the source code doesn't change ...

The idea is more about testing the compilers than anything else, but I'm fine with leaving it as it is.
For example things such as this: ldc-developers/druntime@16d80d5 o_O (from ldc-developers/ldc#2076).

Issues like that one easily hide in the shadows until someone hits them somewhere deep in their non-trivial codebase.

PetarKirov · 2017-06-25T12:06:15Z

I decided to move compiler testing discussion to different PR/issue in the interest of time, as the core of the PR was complete.

wilzbach added 2 commits June 25, 2017 10:00

Enable DScanner checking on Travis

724d617

Fix linter warnings

f7358c4

wilzbach commented Jun 25, 2017

View reviewed changes

PetarKirov reviewed Jun 25, 2017

View reviewed changes

wilzbach added 3 commits June 25, 2017 13:00

Set opequals_tohash_check to disabled

8cfa931

Set could_be_immutable to 'disabled'

cb52192

Split expression back into multiple lines

5dcb5ed

wilzbach commented Jun 25, 2017

View reviewed changes

PetarKirov reviewed Jun 25, 2017

View reviewed changes

wilzbach added 2 commits June 25, 2017 13:47

Set style to disabled

8ef88bf

Fix Makefile target

7429a1a

PetarKirov merged commit 61d5215 into dlang-community:master Jun 25, 2017

wilzbach deleted the enable-dscanner branch July 1, 2017 20:12

Comments

Conversation

wilzbach commented Jun 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov 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!

PetarKirov 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!

PetarKirov 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!

PetarKirov 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!

PetarKirov 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!

PetarKirov 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!

wilzbach commented Jun 25, 2017

Uh oh!

PetarKirov commented Jun 25, 2017

Uh oh!

PetarKirov commented Jun 25, 2017

Uh oh!

PetarKirov 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!

wilzbach commented Jun 25, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov Jun 25, 2017 •

edited

Loading

PetarKirov commented Jun 25, 2017 •

edited

Loading

PetarKirov commented Jun 25, 2017 •

edited

Loading